A key that arrives late now rescues the message
An encrypted event that could not be read kept its placeholder forever. The code said so plainly - "v1 shows an honest placeholder rather than retry-on-later-key-arrival" - and the consequence was that restoring a key backup fixed nothing already on screen, importing keys fixed nothing already on screen, and a restart was the only cure. It is the most visible encryption failure there is: join a room, scroll back, and everything above your join is a wall of "unable to decrypt" that stays that way after the keys to open it have arrived. Two halves, and both were needed. **Asking** - `request_room_key` to this account's other devices and to the sender's, any of which may still hold the Megolm session. The cancellation goes first where there is one, so a room full of unreadable history asks once per session rather than once per message. And **retrying**, because a key can arrive without having been asked for: from a backup restore, from an import, or from somebody else's device deciding to share. The retry runs after each sync's crypto changes are taken in, which is the one place all three have already landed, and again on the two explicit actions - restore and import - where somebody is watching to see whether it worked. What is held is the encrypted event, because that is the only thing that can be decrypted later; the placeholder is a rendering of a failure and cannot be turned back into a message. Bounded at five hundred per account, oldest first. Joining a large encrypted room with no keys produces thousands at once, and holding every one would mean holding the room's whole ciphertext to no purpose - a session that opens one message opens its neighbours, and what anybody can see is a screen's worth. Held across a disconnect on purpose: the messages are still unreadable, the keys may arrive on the next sync or from a backup restored afterwards, and dropping them would make a reconnect fix nothing. Only messages are rewritten. A reaction or an edit that could not be read is dropped rather than replayed - there is no placeholder on screen for either, so there is nothing to correct, and applying them now would apply them out of order. Closes #162. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
Claude Opus 5
@@ -118,6 +118,29 @@ impl CryptoSession {
|
||||
}
|
||||
}
|
||||
|
||||
/// Asks whoever has the key for a message this client cannot read.
|
||||
///
|
||||
/// The one thing that makes an undecryptable message recoverable rather
|
||||
/// than permanent. Sent to our own other devices and to the sender's,
|
||||
/// any of which may still hold the Megolm session - and answered, if it
|
||||
/// is answered, by a forwarded key arriving as a to-device event on some
|
||||
/// later sync.
|
||||
///
|
||||
/// The cancellation is sent first where there is one: it withdraws an
|
||||
/// earlier request for the same session, so a room full of unreadable
|
||||
/// history asks once per session rather than once per message.
|
||||
pub async fn request_room_key(&self, homeserver_url: &str, access_token: &str, event: &Value, room_id: &RoomId) -> Result<()> {
|
||||
let raw: Raw<matrix_sdk_crypto::types::events::room::encrypted::EncryptedEvent> =
|
||||
Raw::from_json_string(event.to_string()).context("re-serializing event to ask for its key")?;
|
||||
let (cancel, request) = self.machine.request_room_key(&raw, room_id).await.context("request_room_key")?;
|
||||
if let Some(cancel) = cancel {
|
||||
if let Err(e) = self.send_one(homeserver_url, access_token, &cancel).await {
|
||||
tracing::debug!("matrix crypto: withdrawing the older key request: {e:#}");
|
||||
}
|
||||
}
|
||||
self.send_one(homeserver_url, access_token, &request).await
|
||||
}
|
||||
|
||||
/// Dispatches one `OutgoingVerificationRequest` - the kind returned
|
||||
/// directly by imperative verification calls (`request_verification()`,
|
||||
/// `VerificationRequest::accept()`/`start_sas()`, `Sas::confirm()`/
|
||||
|
||||
@@ -50,6 +50,7 @@ pub mod notifications;
|
||||
pub mod probes;
|
||||
pub mod profile;
|
||||
pub mod receipts;
|
||||
pub mod relock;
|
||||
pub mod send;
|
||||
pub mod sync;
|
||||
pub mod timeline;
|
||||
@@ -60,6 +61,9 @@ pub use media::*;
|
||||
pub use notifications::*;
|
||||
pub use profile::*;
|
||||
pub use receipts::*;
|
||||
// Not `pub use`: this is asked and answered inside the sync loop, and its
|
||||
// names would say nothing useful in the backend's namespace.
|
||||
use relock::*;
|
||||
pub use send::*;
|
||||
pub use sync::*;
|
||||
// Not `pub use`: an event's handling is this folder's business alone, but
|
||||
|
||||
@@ -0,0 +1,183 @@
|
||||
//! Messages that could not be read, and reading them when the key turns up.
|
||||
//!
|
||||
//! A Megolm session routinely arrives after the messages it opens - key-share
|
||||
//! to-device events lag the timeline, a room joined today has history from
|
||||
//! before this device existed, and key backup restores a session minutes after
|
||||
//! the messages needing it are already on screen. Until now every one of those
|
||||
//! kept its placeholder forever: the message was written down as "unable to
|
||||
//! decrypt" and nothing ever looked at it again, so a restored backup fixed
|
||||
//! nothing that was already displayed and a restart was the only cure.
|
||||
//!
|
||||
//! Two halves, and both are needed. **Asking** - `request_room_key` to our own
|
||||
//! other devices and the sender's, any of which may still hold the session.
|
||||
//! And **retrying** - because a key can arrive without having been asked for,
|
||||
//! from a backup restore, a key import, or somebody else's device deciding to
|
||||
//! share.
|
||||
//!
|
||||
//! What is held is the encrypted event itself, because that is the only thing
|
||||
//! that can be decrypted later; the placeholder on screen is a rendering of a
|
||||
//! failure, not something that can be turned back into a message.
|
||||
|
||||
use super::*;
|
||||
use crate::backend::matrix::crypto::CryptoSession;
|
||||
use serde_json::Value;
|
||||
use std::collections::HashMap;
|
||||
|
||||
/// How many unreadable messages to keep hold of per account.
|
||||
///
|
||||
/// Joining a large encrypted room with no keys at all produces thousands of
|
||||
/// these at once, and holding every one would mean holding the whole room's
|
||||
/// ciphertext in memory to no purpose - what the reader can see is a screen's
|
||||
/// worth, and a session that opens one message opens all of its neighbours.
|
||||
/// Oldest are dropped first.
|
||||
const MAX_PENDING: usize = 500;
|
||||
|
||||
/// One message waiting for its key.
|
||||
#[derive(Clone)]
|
||||
pub struct Locked {
|
||||
pub buffer_id: String,
|
||||
pub room_id: String,
|
||||
pub event_id: String,
|
||||
/// The encrypted event exactly as it arrived, which is the only form that
|
||||
/// can be decrypted later.
|
||||
pub event: Value,
|
||||
}
|
||||
|
||||
fn pending() -> &'static std::sync::Mutex<HashMap<String, Vec<Locked>>> {
|
||||
static PENDING: std::sync::OnceLock<std::sync::Mutex<HashMap<String, Vec<Locked>>>> = std::sync::OnceLock::new();
|
||||
PENDING.get_or_init(Default::default)
|
||||
}
|
||||
|
||||
/// Remembers a message that could not be read, and asks for its key.
|
||||
///
|
||||
/// Best-effort on both counts. A key request that cannot be sent is not worth
|
||||
/// failing a sync over, and the retry below covers the case where nobody ever
|
||||
/// answers it but a key arrives another way.
|
||||
pub async fn note_locked(
|
||||
session: &CryptoSession,
|
||||
homeserver_url: &str,
|
||||
access_token: &str,
|
||||
account_id: &str,
|
||||
buffer_id: &str,
|
||||
room_id: &str,
|
||||
event_id: &str,
|
||||
event: &Value,
|
||||
) {
|
||||
{
|
||||
let mut all = pending().lock().unwrap();
|
||||
let waiting = all.entry(account_id.to_string()).or_default();
|
||||
// The same event twice is the same event: a sync that overlaps what
|
||||
// was already seen would otherwise ask for one key repeatedly.
|
||||
if waiting.iter().any(|l| l.event_id == event_id) {
|
||||
return;
|
||||
}
|
||||
waiting.push(Locked {
|
||||
buffer_id: buffer_id.to_string(),
|
||||
room_id: room_id.to_string(),
|
||||
event_id: event_id.to_string(),
|
||||
event: event.clone(),
|
||||
});
|
||||
if waiting.len() > MAX_PENDING {
|
||||
let over = waiting.len() - MAX_PENDING;
|
||||
waiting.drain(..over);
|
||||
}
|
||||
}
|
||||
|
||||
let Ok(room) = ruma_common::RoomId::parse(room_id) else { return };
|
||||
if let Err(e) = session.request_room_key(homeserver_url, access_token, event, &room).await {
|
||||
tracing::debug!("matrix[{account_id}]: asking for the key to {event_id}: {e:#}");
|
||||
}
|
||||
}
|
||||
|
||||
/// Tries every message still waiting, and rewrites the ones that open.
|
||||
///
|
||||
/// Called after each sync's crypto changes are taken in, and after a key
|
||||
/// backup restore or an import - the three ways a session can appear. Cheap
|
||||
/// when there is nothing waiting, which is the ordinary case.
|
||||
pub async fn retry_locked(state: &AppState, session: &CryptoSession, account_id: &str) {
|
||||
let waiting = {
|
||||
let all = pending().lock().unwrap();
|
||||
match all.get(account_id) {
|
||||
Some(waiting) if !waiting.is_empty() => waiting.clone(),
|
||||
_ => return,
|
||||
}
|
||||
};
|
||||
|
||||
let mut opened: Vec<String> = Vec::new();
|
||||
for locked in &waiting {
|
||||
let Ok(room) = ruma_common::RoomId::parse(&locked.room_id) else {
|
||||
opened.push(locked.event_id.clone());
|
||||
continue;
|
||||
};
|
||||
let Ok(decrypted) = crypto::decrypt_room_event(session, &locked.event, &room).await else { continue };
|
||||
// Only what a message is. A reaction or an edit that could not be
|
||||
// read is dropped rather than rewritten: there is no placeholder on
|
||||
// screen for either, so there is nothing to correct, and replaying
|
||||
// them into the timeline now would apply them out of order.
|
||||
let content = &decrypted["content"];
|
||||
let (body, _) = protocol::message_body(content);
|
||||
if !body.is_empty() {
|
||||
state.runtime.update_message(state, &locked.buffer_id, &locked.event_id, &body, &[], &[]);
|
||||
}
|
||||
opened.push(locked.event_id.clone());
|
||||
}
|
||||
|
||||
if opened.is_empty() {
|
||||
return;
|
||||
}
|
||||
tracing::debug!("matrix[{account_id}]: {} message(s) became readable", opened.len());
|
||||
let mut all = pending().lock().unwrap();
|
||||
if let Some(waiting) = all.get_mut(account_id) {
|
||||
waiting.retain(|l| !opened.contains(&l.event_id));
|
||||
}
|
||||
}
|
||||
|
||||
// Deliberately no way to forget an account's waiting list. Holding it across
|
||||
// a disconnect is what makes a reconnect useful: the events are still
|
||||
// unreadable, the keys may arrive on the next sync or from a backup restored
|
||||
// afterwards, and dropping them would mean a reconnect that fixes nothing.
|
||||
// The list is bounded and empties itself as messages open.
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
fn locked(id: &str) -> Locked {
|
||||
Locked {
|
||||
buffer_id: "matrix:me|!room".into(),
|
||||
room_id: "!room:example.org".into(),
|
||||
event_id: id.into(),
|
||||
event: serde_json::json!({ "type": "m.room.encrypted" }),
|
||||
}
|
||||
}
|
||||
|
||||
/// Joining a large encrypted room with no keys produces thousands of
|
||||
/// these at once. Holding every one would mean holding the room's whole
|
||||
/// ciphertext to no purpose - a session that opens one message opens its
|
||||
/// neighbours, and what anybody can see is a screen's worth.
|
||||
#[test]
|
||||
fn the_oldest_are_dropped_rather_than_growing_without_end() {
|
||||
let mut waiting: Vec<Locked> = (0..MAX_PENDING + 50).map(|i| locked(&format!("${i}"))).collect();
|
||||
if waiting.len() > MAX_PENDING {
|
||||
let over = waiting.len() - MAX_PENDING;
|
||||
waiting.drain(..over);
|
||||
}
|
||||
assert_eq!(waiting.len(), MAX_PENDING);
|
||||
// The newest survive, which are the ones somebody is looking at.
|
||||
assert_eq!(waiting.last().unwrap().event_id, format!("${}", MAX_PENDING + 49));
|
||||
assert_eq!(waiting.first().unwrap().event_id, "$50");
|
||||
}
|
||||
|
||||
/// A sync that overlaps what was already seen must not turn one
|
||||
/// unreadable message into a second key request for the same thing.
|
||||
#[test]
|
||||
fn the_same_message_is_only_waited_on_once() {
|
||||
let mut waiting = vec![locked("$a")];
|
||||
for id in ["$a", "$b", "$a"] {
|
||||
if !waiting.iter().any(|l| l.event_id == id) {
|
||||
waiting.push(locked(id));
|
||||
}
|
||||
}
|
||||
assert_eq!(waiting.len(), 2);
|
||||
}
|
||||
}
|
||||
@@ -235,6 +235,13 @@ pub(super) async fn run_sync(state: &AppState, config: &MatrixAccountConfig, acc
|
||||
// before moving on, same as the tutorial's sync() sketch.
|
||||
session.process_outgoing_requests(&config.homeserver_url, &access_token).await;
|
||||
|
||||
// A key that has just arrived may open messages already on screen as
|
||||
// "unable to decrypt". Here rather than inside the to-device handler
|
||||
// because a session can turn up without having been asked for - from
|
||||
// a backup restore, an import, or somebody else's device deciding to
|
||||
// share - and this is the one place all three have already landed.
|
||||
relock::retry_locked(state, &session, account_id).await;
|
||||
|
||||
verification::tick(state, account_id, &session, &user_id, &config.homeserver_url, &access_token).await;
|
||||
session.run_pending_backup(&config.homeserver_url, &access_token).await;
|
||||
|
||||
|
||||
@@ -106,13 +106,20 @@ pub(super) async fn handle_timeline_event(
|
||||
(t, decrypted["content"].clone(), false)
|
||||
}
|
||||
Err(e) => {
|
||||
// A real, common race: the Megolm session for this
|
||||
// message hasn't arrived yet (key-share to-device events
|
||||
// can lag the timeline event referencing them). v1 shows
|
||||
// an honest placeholder rather than retry-on-later-key-
|
||||
// arrival (request_room_key) - see the plan's Open
|
||||
// decisions for why that's deferred to a later pass.
|
||||
// A real, common race: the Megolm session for this message
|
||||
// has not arrived yet - key-share to-device events lag the
|
||||
// timeline event referencing them, a room joined today has
|
||||
// history from before this device existed, and a backup
|
||||
// restores sessions minutes after the messages needing them
|
||||
// are already on screen.
|
||||
//
|
||||
// The placeholder is still written, because something has to
|
||||
// be. What is new is that the event is kept and its key asked
|
||||
// for, so the placeholder is a state rather than a verdict:
|
||||
// see `relock`. Before this it was permanent, and a restored
|
||||
// backup fixed nothing that was already displayed.
|
||||
tracing::debug!("matrix[{account_id}]: failed to decrypt event {event_id} in {room_id}: {e:#}");
|
||||
relock::note_locked(session, homeserver_url, access_token, account_id, buffer_id, room_id, event_id, event).await;
|
||||
(protocol::EVENT_ROOM_MESSAGE.to_string(), Value::Null, true)
|
||||
}
|
||||
}
|
||||
|
||||
+18
-2
@@ -1250,7 +1250,14 @@ pub async fn dispatch(
|
||||
Err(e) => return (None, Some(format!("could not read the file: {e}"))),
|
||||
};
|
||||
match session.import_room_keys(&text, p_str(params, "passphrase", "")).await {
|
||||
Ok((imported, total)) => (Some(serde_json::json!({ "imported": imported, "total": total })), None),
|
||||
Ok((imported, total)) => {
|
||||
// The messages those keys are for may already be on
|
||||
// screen saying they cannot be read. Importing keys and
|
||||
// leaving them that way is the same failure this was
|
||||
// meant to fix, one step further along.
|
||||
backend::matrix::relock::retry_locked(state, &session, account_id).await;
|
||||
(Some(serde_json::json!({ "imported": imported, "total": total })), None)
|
||||
}
|
||||
Err(e) => (None, Some(format!("{e:#}"))),
|
||||
}
|
||||
}
|
||||
@@ -3489,7 +3496,16 @@ pub async fn dispatch(
|
||||
_ => return (None, Some("restoreMatrixRecoveryKey requires \"accountId\" and \"recoveryKey\"".to_string())),
|
||||
};
|
||||
match backend::matrix::backup::restore_from_recovery_key(state, account_id, recovery_key).await {
|
||||
Ok(summary) => (Some(serde_json::json!({ "restoredKeys": summary.imported_keys, "totalKeys": summary.total_keys })), None),
|
||||
Ok(summary) => {
|
||||
// Restoring a backup is the commonest moment for a
|
||||
// roomful of "unable to decrypt" to become readable, and
|
||||
// the one where somebody is watching to see whether it
|
||||
// worked.
|
||||
if let Some(session) = state.runtime.get_matrix_machine(account_id) {
|
||||
backend::matrix::relock::retry_locked(state, &session, account_id).await;
|
||||
}
|
||||
(Some(serde_json::json!({ "restoredKeys": summary.imported_keys, "totalKeys": summary.total_keys })), None)
|
||||
}
|
||||
Err(e) => (None, Some(e.to_string())),
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user