From c79163e5c24d2c4b8cf8b2989f72ecb4f9e9664b Mon Sep 17 00:00:00 2001 From: Erwan Leboucher Date: Sun, 30 Aug 2026 19:21:54 +0200 Subject: [PATCH 01/10] fix(crypto): tolerate unparseable member ids and reject unusable timeouts --- src-tauri/src/matrix_crypto/devices.rs | 27 ++++++++++++++- src-tauri/src/matrix_crypto/rooms.rs | 46 +++++++++++++++++++++----- 2 files changed, 63 insertions(+), 10 deletions(-) diff --git a/src-tauri/src/matrix_crypto/devices.rs b/src-tauri/src/matrix_crypto/devices.rs index bf836581d..2e5537410 100644 --- a/src-tauri/src/matrix_crypto/devices.rs +++ b/src-tauri/src/matrix_crypto/devices.rs @@ -22,7 +22,7 @@ fn device_id(args: &Value, method: &str) -> Result { fn timeout(args: &Value) -> Option { args.get("timeoutSecs") .and_then(Value::as_f64) - .map(Duration::from_secs_f64) + .and_then(|secs| Duration::try_from_secs_f64(secs).ok()) } fn signatures_json(signatures: &Signatures, method: &str) -> Result { @@ -327,3 +327,28 @@ pub async fn invoke( _ => return None, }) } + +#[cfg(test)] +mod tests { + use super::timeout; + use serde_json::json; + use std::time::Duration; + + /// `timeoutSecs` arrives verbatim from the webview, and `Duration::from_secs_f64` + /// panics on negative, non-finite or overflowing values. + #[test] + fn an_unusable_timeout_is_ignored_rather_than_panicking() { + for secs in [-1.0, f64::NAN, f64::INFINITY, 1e300] { + assert_eq!(timeout(&json!({ "timeoutSecs": secs })), None); + } + } + + #[test] + fn a_usable_timeout_is_kept() { + assert_eq!( + timeout(&json!({ "timeoutSecs": 10.0 })), + Some(Duration::from_secs(10)) + ); + assert_eq!(timeout(&json!({})), None); + } +} diff --git a/src-tauri/src/matrix_crypto/rooms.rs b/src-tauri/src/matrix_crypto/rooms.rs index 68a8ad15a..5e14b7ea2 100644 --- a/src-tauri/src/matrix_crypto/rooms.rs +++ b/src-tauri/src/matrix_crypto/rooms.rs @@ -19,17 +19,23 @@ use super::args::{room_id, str_arg}; use super::wasm_enums::{encryption_algorithm as algorithm_to_wasm, request_type}; fn user_ids(args: &Value, method: &str, field: &str) -> Result, String> { - args.get(field) + let raw = args + .get(field) .and_then(Value::as_array) - .ok_or_else(|| format!("{method}: missing array argument `{field}`"))? + .ok_or_else(|| format!("{method}: missing array argument `{field}`"))?; + + let users: Vec = raw .iter() - .map(|id| { - let id = id - .as_str() - .ok_or_else(|| format!("{method}: `{field}` must contain user id strings"))?; - UserId::parse(id).map_err(|e| format!("{method}: bad user id `{id}` in `{field}`: {e}")) - }) - .collect() + .filter_map(Value::as_str) + .filter_map(|id| UserId::parse(id).ok()) + .collect(); + + let skipped = raw.len() - users.len(); + if skipped > 0 { + log::warn!("{method}: skipped {skipped} unparseable ids in `{field}`"); + } + + Ok(users) } fn as_u64(value: &Value) -> Option { @@ -356,6 +362,7 @@ pub async fn invoke( #[cfg(test)] mod tests { + use super::user_ids; use serde_json::json; use matrix_sdk_crypto::CollectStrategy; @@ -446,4 +453,25 @@ mod tests { std::mem::discriminant(&CollectStrategy::OnlyTrustedDevices), ); } + + /// Membership comes from server state and is not validated by the SDK. A single + /// unparseable id used to abort the whole call, which left the user unable to send + /// any encrypted message in that room. `updateTrackedUsers` and `queryKeysForUsers` + /// already skip the same ids from the same array. + #[test] + fn an_unparseable_member_id_is_skipped_not_fatal() { + let args = json!({ "users": ["@good:example.org", "@bad:under_score", "not-an-id", 7] }); + + let users = user_ids(&args, "shareRoomKey", "users").unwrap(); + + assert_eq!(users.len(), 1); + assert_eq!(users[0].as_str(), "@good:example.org"); + } + + #[test] + fn a_missing_users_array_is_still_an_error() { + let args = json!({}); + + assert!(user_ids(&args, "shareRoomKey", "users").is_err()); + } } From 6f6154dc7e95d1a80c59276188dd871374fae729 Mon Sep 17 00:00:00 2001 From: Erwan Leboucher Date: Sun, 30 Aug 2026 19:25:43 +0200 Subject: [PATCH 02/10] fix(crypto): make opening and releasing the engine atomic across the push and webview paths --- src-tauri/src/matrix_crypto/mod.rs | 68 ++++++++++++++++++++++++----- src-tauri/src/matrix_crypto/push.rs | 11 ++++- 2 files changed, 67 insertions(+), 12 deletions(-) diff --git a/src-tauri/src/matrix_crypto/mod.rs b/src-tauri/src/matrix_crypto/mod.rs index e79387150..a92f0d828 100644 --- a/src-tauri/src/matrix_crypto/mod.rs +++ b/src-tauri/src/matrix_crypto/mod.rs @@ -79,20 +79,33 @@ impl CryptoEngineState { } pub fn close_account_if(&self, account: &str, machine: &Arc) -> Result<(), String> { - let registered = self - .machines + let removed = { + let mut machines = self.machines.lock().map_err(|e| e.to_string())?; + match machines.get(account) { + Some(current) if Arc::ptr_eq(current, machine) => { + machines.remove(account); + true + } + _ => false, + } + }; + + if !removed { + return Ok(()); + } + + if let Some(listeners) = self + .listeners .lock() .map_err(|e| e.to_string())? - .get(account) - .cloned(); - - match registered { - Some(current) if Arc::ptr_eq(¤t, machine) => { - self.close_account(account)?; - Ok(()) + .remove(account) + { + for listener in listeners { + listener.abort(); } - _ => Ok(()), } + + Ok(()) } } @@ -168,6 +181,15 @@ pub async fn open_machine( device_id: &str, ) -> Result<(Arc, EngineInfo), String> { let _guard = OPEN_GUARD.lock().await; + open_machine_locked(dir, passphrase, user_id, device_id).await +} + +pub(super) async fn open_machine_locked( + dir: &Path, + passphrase: Option<&str>, + user_id: &str, + device_id: &str, +) -> Result<(Arc, EngineInfo), String> { let user: &matrix_sdk::ruma::UserId = user_id .try_into() .map_err(|e| format!("bad user id: {e}"))?; @@ -300,6 +322,32 @@ mod tests { ); } + /// A cold push and the webview can both be opening the same account. The push must + /// never deregister a machine it did not itself open, or the webview's crypto dies + /// for the rest of the session. + #[tokio::test] + async fn close_account_if_leaves_a_machine_it_does_not_own() { + let user: &matrix_sdk::ruma::UserId = "@race:example.org".try_into().unwrap(); + let device: &matrix_sdk::ruma::DeviceId = "RACEDEVICE".into(); + let account = account_key(user.as_str(), device.as_str()); + + let mine = Arc::new(OlmMachine::new(user, device).await); + let theirs = Arc::new(OlmMachine::new(user, device).await); + + let state = CryptoEngineState::default(); + state + .machines + .lock() + .unwrap() + .insert(account.clone(), Arc::clone(&theirs)); + + state.close_account_if(&account, &mine).unwrap(); + assert!(state.machine(user.as_str(), device.as_str()).is_ok()); + + state.close_account_if(&account, &theirs).unwrap(); + assert!(state.machine(user.as_str(), device.as_str()).is_err()); + } + #[tokio::test] async fn engine_plumbing() { let dir = std::env::temp_dir().join(format!("sable-engine-test-{}", std::process::id())); diff --git a/src-tauri/src/matrix_crypto/push.rs b/src-tauri/src/matrix_crypto/push.rs index 6246c31c9..aac0c7956 100644 --- a/src-tauri/src/matrix_crypto/push.rs +++ b/src-tauri/src/matrix_crypto/push.rs @@ -10,7 +10,7 @@ use matrix_sdk_crypto::OlmMachine; use serde_json::Value; use super::args::decryption_settings; -use super::{account_key, engines, open_machine}; +use super::{account_key, engines}; /// Returns the machine already registered for the account, opening one if the process is /// cold, and reports whether this call is what opened the store. Never evicts a machine @@ -25,7 +25,13 @@ pub async fn open_machine_for_push( return Ok((machine, false)); } - let (machine, _) = open_machine(dir, passphrase, user_id, device_id).await?; + let _guard = super::OPEN_GUARD.lock().await; + + if let Ok(machine) = engines().machine(user_id, device_id) { + return Ok((machine, false)); + } + + let (machine, _) = super::open_machine_locked(dir, passphrase, user_id, device_id).await?; Ok((machine, true)) } @@ -128,6 +134,7 @@ mod tests { use matrix_sdk_sqlite::SqliteCryptoStore; use serde_json::json; + use super::super::open_machine; use super::*; fn temp_dir(name: &str) -> std::path::PathBuf { From 4097e090be23de1974c537ff07133389dc064148 Mon Sep 17 00:00:00 2001 From: Erwan Leboucher Date: Sun, 30 Aug 2026 19:33:07 +0200 Subject: [PATCH 03/10] fix(crypto): match upstream on to-device drops, room-key forwarding and unknown devices --- src-tauri/Cargo.toml | 1 + src-tauri/src/matrix_crypto/devices.rs | 4 +-- src-tauri/src/matrix_crypto/dispatch.rs | 36 ++++++++++----------- src-tauri/src/matrix_crypto/events.rs | 13 +++++--- src/app/crypto/engineCrypto/EngineCrypto.ts | 2 +- 5 files changed, 30 insertions(+), 26 deletions(-) diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index f26aa20d9..7fca93c07 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -70,6 +70,7 @@ matrix-sdk = { version = "0.18", default-features = false, features = [ # verification, so the engine needs it for parity with the wasm backend. matrix-sdk-crypto = { version = "0.18", features = [ "qrcode", + "automatic-room-key-forwarding", "experimental-push-secrets", ], optional = true } # `bundled` compiles SQLite from source. Android's NDK ships no libsqlite3 to link diff --git a/src-tauri/src/matrix_crypto/devices.rs b/src-tauri/src/matrix_crypto/devices.rs index 2e5537410..afa75758c 100644 --- a/src-tauri/src/matrix_crypto/devices.rs +++ b/src-tauri/src/matrix_crypto/devices.rs @@ -195,7 +195,7 @@ fn query_keys_for_users(machine: &OlmMachine, args: &Value, method: &str) -> Res async fn verify_device(machine: &OlmMachine, args: &Value, method: &str) -> Result { let Some(device) = device_for(machine, args, method).await? else { - return Ok(Value::Null); + return Err(format!("{method}: unknown device")); }; let request = device .verify() @@ -214,7 +214,7 @@ async fn set_local_trust( .and_then(Value::as_i64) .ok_or_else(|| format!("{method}: missing numeric argument `trustState`"))?; let Some(device) = device_for(machine, args, method).await? else { - return Ok(Value::Null); + return Err(format!("{method}: unknown device")); }; device .set_local_trust(LocalTrust::from(trust)) diff --git a/src-tauri/src/matrix_crypto/dispatch.rs b/src-tauri/src/matrix_crypto/dispatch.rs index c1779da0d..bb6e4a22c 100644 --- a/src-tauri/src/matrix_crypto/dispatch.rs +++ b/src-tauri/src/matrix_crypto/dispatch.rs @@ -76,7 +76,7 @@ enum ToDeviceEncryptionInfoSnapshot { fn processed_to_device_event_json( event: &ProcessedToDeviceEvent, verification_request: Option, -) -> Result { +) -> Option { let raw_event = event.as_raw().json().get(); let snapshot = match event { @@ -88,10 +88,10 @@ fn processed_to_device_event_json( curve25519_public_key_base64, } => curve25519_public_key_base64.as_str(), _ => { - return Err( - "receiveSyncChanges: decrypted to-device event did not use Olm v1" - .to_owned(), - ) + log::warn!( + "Dropping incoming to-device event with unrecognised encryption_info" + ); + return None; } }; @@ -132,12 +132,11 @@ fn processed_to_device_event_json( } }; - let mut value = serde_json::to_value(snapshot) - .map_err(|e| format!("receiveSyncChanges: failed to serialize processed event: {e}"))?; + let mut value = serde_json::to_value(snapshot).ok()?; if let Some(request) = verification_request { value["verificationRequest"] = request; } - Ok(value) + Some(value) } mod decryption_error_code { @@ -298,16 +297,17 @@ pub async fn invoke(machine: &OlmMachine, method: &str, args: Value) -> Result, _>>() - .map(Value::Array) + Ok(Value::Array( + processed + .iter() + .filter_map(|event| { + processed_to_device_event_json( + event, + verification_request_snapshot(machine, event), + ) + }) + .collect(), + )) } "outgoingRequests" => outgoing_requests(machine).await, diff --git a/src-tauri/src/matrix_crypto/events.rs b/src-tauri/src/matrix_crypto/events.rs index e776941a1..e88081b75 100644 --- a/src-tauri/src/matrix_crypto/events.rs +++ b/src-tauri/src/matrix_crypto/events.rs @@ -59,11 +59,14 @@ pub fn spawn( let (app, account) = (app.clone(), account.clone()); async move { while let Some(update) = room_keys.next().await { - // Lagging drops updates rather than ending the stream; js-sdk - // recovers on the next key or a retry, so keep listening. - if let Ok(keys) = update { - let payload = Value::Array(keys.iter().map(room_key_json).collect()); - emit(app.clone(), ROOM_KEYS_RECEIVED, account.clone(), payload); + match update { + Ok(keys) => { + let payload = Value::Array(keys.iter().map(room_key_json).collect()); + emit(app.clone(), ROOM_KEYS_RECEIVED, account.clone(), payload); + } + Err(error) => { + log::warn!("room-key stream lagged, {error}; some events may stay undecryptable until restart"); + } } } } diff --git a/src/app/crypto/engineCrypto/EngineCrypto.ts b/src/app/crypto/engineCrypto/EngineCrypto.ts index d24ccdf4d..a6559845f 100644 --- a/src/app/crypto/engineCrypto/EngineCrypto.ts +++ b/src/app/crypto/engineCrypto/EngineCrypto.ts @@ -1388,7 +1388,7 @@ export class EngineCrypto userId: string = this.#identity.userId, downloadUncached = false ): Promise { - if (downloadUncached) { + if (downloadUncached || userId === this.#identity.userId) { await this.#sendTracked(await this.#call('queryKeysForUsers', { users: [userId] })); } const identity = (await this.#call('getIdentity', { userId })) as EngineIdentityInfo | null; From 0391e50db2d98ba14cd1fae7af5e8c277259c3bf Mon Sep 17 00:00:00 2001 From: Erwan Leboucher Date: Sun, 30 Aug 2026 19:35:51 +0200 Subject: [PATCH 04/10] fix(crypto): match upstream on backup completion, secret storage writes and gossip errors --- src/app/crypto/engineCrypto/EngineCrypto.ts | 18 ++++++-- .../crypto/engineCrypto/backupUpload.test.ts | 42 +++++++++++++++---- src/app/crypto/engineCrypto/eventBridge.ts | 7 +++- 3 files changed, 53 insertions(+), 14 deletions(-) diff --git a/src/app/crypto/engineCrypto/EngineCrypto.ts b/src/app/crypto/engineCrypto/EngineCrypto.ts index a6559845f..c63391380 100644 --- a/src/app/crypto/engineCrypto/EngineCrypto.ts +++ b/src/app/crypto/engineCrypto/EngineCrypto.ts @@ -378,7 +378,10 @@ export class EngineCrypto if (this.#stopped) return; // eslint-disable-next-line no-await-in-loop const request = (await this.#call('backupRoomKeys')) as OutgoingRequest | null; - if (!request) break; + if (!request) { + this.emit(CryptoEvent.KeyBackupSessionsRemaining, 0); + return; + } try { // eslint-disable-next-line no-await-in-loop @@ -402,8 +405,6 @@ export class EngineCrypto const counts = (await this.#call('roomKeyCounts')) as { total: number; backedUp: number }; this.emit(CryptoEvent.KeyBackupSessionsRemaining, counts.total - counts.backedUp); } - - this.emit(CryptoEvent.KeyBackupSessionsRemaining, 0); } async #recoverFromBackupUploadError(error: unknown): Promise { @@ -2003,7 +2004,16 @@ export class EngineCrypto await this.#call('enableBackupV1', { publicKeyBase64: publicKey, version: created.version }); await this.storeSessionBackupPrivateKey(key.privateKey, created.version); await this.#pushSecretToVerifiedDevices('m.megolm_backup.v1'); - await this.#mx.secretStorage.store('m.megolm_backup.v1', encodeBase64(key.privateKey)); + if (await this.#secretStorageHasAesKey()) { + await this.#mx.secretStorage.store('m.megolm_backup.v1', encodeBase64(key.privateKey)); + } + } + + async #secretStorageHasAesKey(): Promise { + const stored = await this.#mx.secretStorage.getKey(); + if (!stored) return false; + const [, keyInfo] = stored; + return keyInfo.algorithm === SECRET_STORAGE_ALGORITHM_V1_AES; } async #signatureFor(value: Record): Promise | null> { diff --git a/src/app/crypto/engineCrypto/backupUpload.test.ts b/src/app/crypto/engineCrypto/backupUpload.test.ts index 2700aa499..684fed377 100644 --- a/src/app/crypto/engineCrypto/backupUpload.test.ts +++ b/src/app/crypto/engineCrypto/backupUpload.test.ts @@ -1,6 +1,7 @@ import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; import * as RustSdkCryptoJs from '@matrix-org/matrix-sdk-crypto-wasm'; import { CryptoEvent } from 'matrix-js-sdk/lib/crypto-api'; +import { SECRET_STORAGE_ALGORITHM_V1_AES } from 'matrix-js-sdk/lib/secret-storage'; import type { MatrixClient } from '$types/matrix-sdk'; import { engineInvoke } from '../olmMachine/engineInvoke'; import { EngineCrypto } from './EngineCrypto'; @@ -109,6 +110,20 @@ describe('key backup upload', () => { }); }); +function clientWithSecretStorage(key: unknown) { + const store = vi.fn<(name: string, value: string) => Promise>(async () => undefined); + const getKey = vi.fn<() => Promise>(async () => key); + const mx = { + http: { + authedRequest: vi.fn<(...args: never[]) => Promise>(async () => ({ + version: '8', + })), + }, + secretStorage: { store, getKey }, + } as unknown as MatrixClient; + return { mx, store }; +} + describe('resetKeyBackup', () => { beforeAll(() => RustSdkCryptoJs.initAsync()); @@ -120,15 +135,10 @@ describe('resetKeyBackup', () => { if (method === 'backupVersion') return null; return null; }); - const store = vi.fn<(name: string, value: string) => Promise>(async () => undefined); - const mx = { - http: { - authedRequest: vi.fn<(...args: never[]) => Promise>(async () => ({ - version: '8', - })), - }, - secretStorage: { store }, - } as unknown as MatrixClient; + const { mx, store } = clientWithSecretStorage([ + 'key-id', + { algorithm: SECRET_STORAGE_ALGORITHM_V1_AES }, + ]); await new EngineCrypto(mx, { userId: '@me:e.org', deviceId: 'D' }).resetKeyBackup(); @@ -142,4 +152,18 @@ describe('resetKeyBackup', () => { expect(invoked('getMissingSessions')[0]?.[2]).toMatchObject({ users: ['@me:e.org'] }); expect(store).toHaveBeenCalledWith('m.megolm_backup.v1', expect.any(String)); }); + + it('does not write the backup key to secret storage when 4S is not set up', async () => { + mockInvoke.mockImplementation(async (_identity, method) => { + if (method === 'isBackupEnabled') return false; + if (method === 'backupVersion') return null; + return null; + }); + const { mx, store } = clientWithSecretStorage(null); + + await new EngineCrypto(mx, { userId: '@me:e.org', deviceId: 'D' }).resetKeyBackup(); + + expect(invoked('pushSecretToVerifiedDevices')).toHaveLength(1); + expect(store).not.toHaveBeenCalled(); + }); }); diff --git a/src/app/crypto/engineCrypto/eventBridge.ts b/src/app/crypto/engineCrypto/eventBridge.ts index 67b331bc9..1edacfb45 100644 --- a/src/app/crypto/engineCrypto/eventBridge.ts +++ b/src/app/crypto/engineCrypto/eventBridge.ts @@ -1,7 +1,10 @@ import { listen, type UnlistenFn } from '@tauri-apps/api/event'; +import { createDebugLogger } from '$utils/debugLogger'; import type { EngineIdentity } from '../olmMachine/engineInvoke'; import type { EngineCrypto } from './EngineCrypto'; +const eventBridgeLog = createDebugLogger('crypto'); + const ROOM_KEYS_RECEIVED = 'matrix-crypto://room-keys-received'; const ROOM_KEYS_WITHHELD = 'matrix-crypto://room-keys-withheld'; const IDENTITIES_UPDATED = 'matrix-crypto://identities-updated'; @@ -43,7 +46,9 @@ export const startCryptoEventBridge = async ( listen>( SECRET_RECEIVED, forAccount<{ name: string }>(({ name }) => { - crypto.checkSecrets(name).catch(() => undefined); + crypto.checkSecrets(name).catch((error: unknown) => { + eventBridgeLog.warn('general', `Failed to handle gossiped secret ${name}`, error); + }); }) ), ]); From 4fb7eeb0e65dc114994f7c16d91214e2ca7cc19c Mon Sep 17 00:00:00 2001 From: Erwan Leboucher Date: Sun, 30 Aug 2026 19:40:45 +0200 Subject: [PATCH 05/10] fix(crypto): keep room key requests disabled and report real restore progress --- src-tauri/src/matrix_crypto/backup.rs | 7 ++-- src-tauri/src/matrix_crypto/mod.rs | 22 +++++++++++++ src/app/crypto/engineCrypto/EngineCrypto.ts | 32 +++++++++++++------ .../crypto/engineCrypto/backupImport.test.ts | 22 ++++++++++--- 4 files changed, 66 insertions(+), 17 deletions(-) diff --git a/src-tauri/src/matrix_crypto/backup.rs b/src-tauri/src/matrix_crypto/backup.rs index d406bf5b3..ebff84879 100644 --- a/src-tauri/src/matrix_crypto/backup.rs +++ b/src-tauri/src/matrix_crypto/backup.rs @@ -66,10 +66,11 @@ fn exportable_keys(sessions: Vec) -> (Vec, usi (exported, skipped) } -fn import_result(result: RoomKeyImportResult) -> Value { +fn import_result(result: RoomKeyImportResult, skipped: usize) -> Value { json!({ "importedCount": result.imported_count, "totalCount": result.total_count, + "skippedCount": skipped, "keys": result.keys, }) } @@ -185,7 +186,7 @@ async fn handle(machine: &OlmMachine, method: &str, args: &Value) -> Result { let keys: Vec = serde_json::from_str(&str_arg(args, method, "keys")?) @@ -195,7 +196,7 @@ async fn handle(machine: &OlmMachine, method: &str, args: &Value) -> Result { let keys = machine diff --git a/src-tauri/src/matrix_crypto/mod.rs b/src-tauri/src/matrix_crypto/mod.rs index a92f0d828..6f0142910 100644 --- a/src-tauri/src/matrix_crypto/mod.rs +++ b/src-tauri/src/matrix_crypto/mod.rs @@ -211,6 +211,8 @@ pub(super) async fn open_machine_locked( .await .map_err(|e| format!("creating OlmMachine failed: {e}"))?, ); + machine.set_room_key_requests_enabled(false); + let keys = machine.identity_keys(); engines() @@ -322,6 +324,26 @@ mod tests { ); } + /// Enabling `automatic-room-key-forwarding` also enables OUTGOING room key + /// requests, which upstream turns off (element-web#26524). The two must stay paired. + #[tokio::test] + async fn outgoing_room_key_requests_stay_disabled() { + let user: &matrix_sdk::ruma::UserId = "@gossip:example.org".try_into().unwrap(); + let device: &matrix_sdk::ruma::DeviceId = "GOSSIPDEVICE".into(); + + let dir = std::env::temp_dir().join(format!("sable-gossip-test-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + + let (machine, _) = open_machine(&dir, None, user.as_str(), device.as_str()) + .await + .unwrap(); + + assert!(!machine.are_room_key_requests_enabled()); + + let _ = engines().close_account(&account_key(user.as_str(), device.as_str())); + let _ = std::fs::remove_dir_all(&dir); + } + /// A cold push and the webview can both be opening the same account. The push must /// never deregister a machine it did not itself open, or the webview's crypto dies /// for the rest of the session. diff --git a/src/app/crypto/engineCrypto/EngineCrypto.ts b/src/app/crypto/engineCrypto/EngineCrypto.ts index c63391380..d512ac6b2 100644 --- a/src/app/crypto/engineCrypto/EngineCrypto.ts +++ b/src/app/crypto/engineCrypto/EngineCrypto.ts @@ -1135,21 +1135,27 @@ export class EngineCrypto backupVersion: string, opts?: ImportRoomKeysOpts, already = 0, - grandTotal = keys.length - ): Promise { + grandTotal = keys.length, + alreadyFailed = 0 + ): Promise<{ imported: number; processed: number; failures: number }> { const result = (await this.#call('importBackedUpRoomKeys', { keys: JSON.stringify(keys), backupVersion, - })) as { importedCount?: number; totalCount?: number } | null; + })) as { importedCount?: number; totalCount?: number; skippedCount?: number } | null; - const successes = already + (result?.importedCount ?? 0); + const processed = already + (result?.totalCount ?? 0); + const failures = alreadyFailed + (result?.skippedCount ?? 0); opts?.progressCallback?.({ stage: ImportRoomKeyStage.LoadKeys, - successes, - failures: grandTotal - successes, + successes: processed, + failures, total: grandTotal, }); - return result?.importedCount ?? 0; + return { + imported: result?.importedCount ?? 0, + processed: result?.totalCount ?? 0, + failures: result?.skippedCount ?? 0, + }; } /** MSC4268. The engine encrypts; we upload; only the mxc URL goes back. */ @@ -2110,6 +2116,8 @@ export class EngineCrypto ); let imported = 0; + let processed = 0; + let failures = 0; for (const [roomId, room] of rooms) { // eslint-disable-next-line no-await-in-loop const decrypted = await decryptor.decryptSessions(room.sessions ?? {}); @@ -2117,13 +2125,17 @@ export class EngineCrypto for (let start = 0; start < withRoom.length; start += RESTORE_CHUNK_SIZE) { // eslint-disable-next-line no-await-in-loop - imported += await this.#importBackedUpRoomKeys( + const chunk = await this.#importBackedUpRoomKeys( withRoom.slice(start, start + RESTORE_CHUNK_SIZE), keys.backupVersion, opts, - imported, - total + processed, + total, + failures ); + imported += chunk.imported; + processed += chunk.processed; + failures += chunk.failures; } } diff --git a/src/app/crypto/engineCrypto/backupImport.test.ts b/src/app/crypto/engineCrypto/backupImport.test.ts index 70e9480a9..f1726b15d 100644 --- a/src/app/crypto/engineCrypto/backupImport.test.ts +++ b/src/app/crypto/engineCrypto/backupImport.test.ts @@ -34,8 +34,22 @@ describe('importBackedUpRoomKeys', () => { expect(JSON.parse(args.keys)).toHaveLength(2); }); - it('reports the counts the engine actually imported, not the counts requested', async () => { - mockInvoke.mockResolvedValueOnce({ importedCount: 1, totalCount: 2 }); + it('counts keys the engine already held as processed, not as failures', async () => { + mockInvoke.mockResolvedValueOnce({ importedCount: 0, totalCount: 2, skippedCount: 0 }); + const progressCallback = vi.fn<(stage: unknown) => void>(); + + await crypto().importBackedUpRoomKeys([session('a'), session('b')], '7', { progressCallback }); + + expect(progressCallback).toHaveBeenCalledWith({ + stage: ImportRoomKeyStage.LoadKeys, + successes: 2, + failures: 0, + total: 2, + }); + }); + + it('reports only the keys the engine could not read as failures', async () => { + mockInvoke.mockResolvedValueOnce({ importedCount: 1, totalCount: 1, skippedCount: 1 }); const progressCallback = vi.fn<(stage: unknown) => void>(); await crypto().importBackedUpRoomKeys([session('a'), session('b')], '7', { progressCallback }); @@ -48,14 +62,14 @@ describe('importBackedUpRoomKeys', () => { }); }); - it('falls back to the requested count when the engine reports nothing', async () => { + it('reports nothing processed when the engine reports nothing', async () => { mockInvoke.mockResolvedValueOnce(null); const progressCallback = vi.fn<(stage: unknown) => void>(); await crypto().importBackedUpRoomKeys([session('a')], '7', { progressCallback }); expect(progressCallback).toHaveBeenCalledWith( - expect.objectContaining({ successes: 0, failures: 1, total: 1 }) + expect.objectContaining({ successes: 0, failures: 0, total: 1 }) ); }); }); From a9eaf558404a2cc6c6efcf0fb8ff51e2e15aa9f7 Mon Sep 17 00:00:00 2001 From: Erwan Leboucher Date: Sun, 30 Aug 2026 19:48:18 +0200 Subject: [PATCH 06/10] fix(crypto): retry outgoing requests and serialize key claims and room encryption --- src/app/crypto/engineCrypto/EngineCrypto.ts | 48 ++++++++++++---- .../engineCrypto/encryptionOrder.test.ts | 57 +++++++++++++++++++ src/app/crypto/engineCrypto/outgoing.ts | 28 ++++++--- .../crypto/engineCrypto/outgoingRetry.test.ts | 43 ++++++++++++++ 4 files changed, 158 insertions(+), 18 deletions(-) create mode 100644 src/app/crypto/engineCrypto/encryptionOrder.test.ts create mode 100644 src/app/crypto/engineCrypto/outgoingRetry.test.ts diff --git a/src/app/crypto/engineCrypto/EngineCrypto.ts b/src/app/crypto/engineCrypto/EngineCrypto.ts index d512ac6b2..0014faaac 100644 --- a/src/app/crypto/engineCrypto/EngineCrypto.ts +++ b/src/app/crypto/engineCrypto/EngineCrypto.ts @@ -331,6 +331,10 @@ export class EngineCrypto readonly #roomsWithTrackedMembers = new Set(); + #claimChain: Promise = Promise.resolve(); + + readonly #encryptionChains = new Map>(); + readonly #backupUpload = createCoalescedRunner( () => this.#uploadRoomKeysToBackup().catch((error: unknown) => { @@ -851,6 +855,8 @@ export class EngineCrypto this.#backupUpload.cancel(); this.#eventsPendingKey.clear(); this.#roomsWithTrackedMembers.clear(); + this.#encryptionChains.clear(); + this.#claimChain = Promise.resolve(); } #trustRequirement(): number { @@ -893,7 +899,30 @@ export class EngineCrypto return settings; } + #serializeForRoom(roomId: string, run: () => Promise): Promise { + const next = (this.#encryptionChains.get(roomId) ?? Promise.resolve()) + .catch(() => undefined) + .then(run); + this.#encryptionChains.set(roomId, next); + return next; + } + + async #ensureSessionsForUsers(users: string[]): Promise { + const next = this.#claimChain + .catch(() => undefined) + .then(async () => { + const claim = (await this.#call('getMissingSessions', { users })) as OutgoingRequest | null; + await this.#sendTracked(claim); + }); + this.#claimChain = next; + await next; + } + async encryptEvent(event: MatrixEvent, room: Room): Promise { + return this.#serializeForRoom(room.roomId, () => this.#encryptEventInner(event, room)); + } + + async #encryptEventInner(event: MatrixEvent, room: Room): Promise { // The megolm session has to reach every device in the room before the event does. const members = await room.getEncryptionTargetMembers(); const users = members.map((member) => member.userId); @@ -906,8 +935,7 @@ export class EngineCrypto this.#roomsWithTrackedMembers.add(room.roomId); } - const claim = (await this.#call('getMissingSessions', { users })) as OutgoingRequest | null; - await this.#sendTracked(claim); + await this.#ensureSessionsForUsers(users); const shared = ((await this.#call('shareRoomKey', { roomId: room.roomId, @@ -1295,15 +1323,13 @@ export class EngineCrypto } prepareToEncrypt(room: Room): void { - void room - .getEncryptionTargetMembers() - .then(async (members) => { - const users = members.map((member) => member.userId); - await this.#trackUsers(users); - await this.#sendTracked(await this.#call('getMissingSessions', { users })); - await this.#flushOutgoingRequests(); - }) - .catch((error: unknown) => engineCryptoLog.warn('general', 'prepareToEncrypt failed', error)); + void this.#serializeForRoom(room.roomId, async () => { + const members = await room.getEncryptionTargetMembers(); + const users = members.map((member) => member.userId); + await this.#trackUsers(users); + await this.#ensureSessionsForUsers(users); + await this.#flushOutgoingRequests(); + }).catch((error: unknown) => engineCryptoLog.warn('general', 'prepareToEncrypt failed', error)); } async forceDiscardSession(roomId: string): Promise { diff --git a/src/app/crypto/engineCrypto/encryptionOrder.test.ts b/src/app/crypto/engineCrypto/encryptionOrder.test.ts new file mode 100644 index 000000000..07d0e3951 --- /dev/null +++ b/src/app/crypto/engineCrypto/encryptionOrder.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it, vi } from 'vitest'; +import type { MatrixClient, MatrixEvent, Room } from '$types/matrix-sdk'; +import { engineInvoke } from '../olmMachine/engineInvoke'; +import { EngineCrypto } from './EngineCrypto'; + +vi.mock('../olmMachine/engineInvoke', () => ({ engineInvoke: vi.fn() })); + +const mockInvoke = vi.mocked(engineInvoke); + +const room = (roomId: string) => + ({ + roomId, + getEncryptionTargetMembers: async () => [{ userId: '@me:e.org' }], + getHistoryVisibility: () => 'shared', + getBlacklistUnverifiedDevices: () => false, + currentState: { getStateEvents: () => null }, + }) as unknown as Room; + +const event = () => + ({ + getType: () => 'm.room.message', + getContent: () => ({}), + makeEncrypted: vi.fn(), + getTxnId: () => 't', + }) as unknown as MatrixEvent; + +describe('encryptEvent ordering', () => { + it('never runs two encryptions for a room at the same time', async () => { + const marks: string[] = []; + let inFlight = 0; + + mockInvoke.mockImplementation(async (_identity, method) => { + if (method === 'encryptRoomEvent') { + inFlight += 1; + marks.push(`enter${inFlight}`); + await new Promise((resolve) => { + setTimeout(resolve, 0); + }); + inFlight -= 1; + marks.push('exit'); + return '{}'; + } + if (method === 'identityKeys') return { ed25519: 'e', curve25519: 'c' }; + return null; + }); + + const mx = { http: { authedRequest: vi.fn() } } as unknown as MatrixClient; + const crypto = new EngineCrypto(mx, { userId: '@me:e.org', deviceId: 'D' }); + + await Promise.all([ + crypto.encryptEvent(event(), room('!r:e.org')), + crypto.encryptEvent(event(), room('!r:e.org')), + ]); + + expect(marks).toEqual(['enter1', 'exit', 'enter1', 'exit']); + }); +}); diff --git a/src/app/crypto/engineCrypto/outgoing.ts b/src/app/crypto/engineCrypto/outgoing.ts index d09b8ac34..9ef7431c3 100644 --- a/src/app/crypto/engineCrypto/outgoing.ts +++ b/src/app/crypto/engineCrypto/outgoing.ts @@ -1,4 +1,6 @@ import { Method } from 'matrix-js-sdk/lib/http-api'; +import { calculateRetryBackoff } from 'matrix-js-sdk/lib/http-api/utils'; +import { sleep } from 'matrix-js-sdk/lib/utils'; import type { MatrixClient } from '$types/matrix-sdk'; /** Numeric codes the engine tags outgoing requests with; see wasm_enums.rs. */ @@ -41,13 +43,25 @@ export const sendOutgoingRequest = async ( mx: MatrixClient, request: OutgoingRequest ): Promise => { - const send = (method: Method, url: string, params: Record = {}) => - mx.http.authedRequest(method, url, params, request.body, { - prefix: '', - json: false, - localTimeoutMs: OUTGOING_REQUEST_TIMEOUT_MS, - headers: { 'Content-Type': 'application/json', Accept: 'application/json' }, - }); + const send = async (method: Method, url: string, params: Record = {}) => { + for (let attempts = 0; ;) { + try { + // eslint-disable-next-line no-await-in-loop + return await mx.http.authedRequest(method, url, params, request.body, { + prefix: '', + json: false, + localTimeoutMs: OUTGOING_REQUEST_TIMEOUT_MS, + headers: { 'Content-Type': 'application/json', Accept: 'application/json' }, + }); + } catch (error) { + attempts += 1; + const backoff = calculateRetryBackoff(error, attempts, true); + if (backoff < 0) throw error; + // eslint-disable-next-line no-await-in-loop + await sleep(backoff); + } + } + }; switch (request.type) { case RequestType.KeysUpload: diff --git a/src/app/crypto/engineCrypto/outgoingRetry.test.ts b/src/app/crypto/engineCrypto/outgoingRetry.test.ts new file mode 100644 index 000000000..370900c2e --- /dev/null +++ b/src/app/crypto/engineCrypto/outgoingRetry.test.ts @@ -0,0 +1,43 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { Method } from 'matrix-js-sdk/lib/http-api'; +import type { MatrixClient } from '$types/matrix-sdk'; +import { RequestType, sendOutgoingRequest } from './outgoing'; + +vi.mock('matrix-js-sdk/lib/utils', async (importOriginal) => ({ + ...(await importOriginal()), + sleep: vi.fn(async () => undefined), +})); + +const rateLimited = () => + Object.assign(new Error('rate limited'), { + httpStatus: 429, + data: { errcode: 'M_LIMIT_EXCEEDED', retry_after_ms: 10 }, + }); + +const request = { id: 'r1', type: RequestType.KeysClaim, body: '{}' }; + +describe('sendOutgoingRequest', () => { + beforeEach(() => vi.clearAllMocks()); + + it('retries a rate-limited request instead of failing the caller', async () => { + const authedRequest = vi + .fn<(...args: never[]) => Promise>() + .mockRejectedValueOnce(rateLimited()) + .mockResolvedValueOnce('{}'); + const mx = { http: { authedRequest } } as unknown as MatrixClient; + + await expect(sendOutgoingRequest(mx, request)).resolves.toBe('{}'); + expect(authedRequest).toHaveBeenCalledTimes(2); + expect(authedRequest.mock.calls[0]?.[0]).toBe(Method.Post); + }); + + it('rethrows an error that is not worth retrying', async () => { + const authedRequest = vi + .fn<(...args: never[]) => Promise>() + .mockRejectedValue(Object.assign(new Error('nope'), { httpStatus: 400, data: {} })); + const mx = { http: { authedRequest } } as unknown as MatrixClient; + + await expect(sendOutgoingRequest(mx, request)).rejects.toThrow('nope'); + expect(authedRequest).toHaveBeenCalledTimes(1); + }); +}); From f1b531a1124e744e3d3b9f0a2cf5d150ebffd34b Mon Sep 17 00:00:00 2001 From: Erwan Leboucher Date: Sun, 30 Aug 2026 19:53:54 +0200 Subject: [PATCH 07/10] fix(crypto): validate the stored backup key and settle a cancelled SAS flow --- src/app/components/DeviceVerification.tsx | 2 +- src/app/crypto/engineCrypto/EngineCrypto.ts | 25 +++++++++- .../engineCrypto/backupKeyFrom4S.test.ts | 50 +++++++++++++++++++ src/app/crypto/verification/request.ts | 6 ++- src/app/crypto/verification/verifier.test.ts | 38 ++++++++++++++ src/app/crypto/verification/verifier.ts | 17 +++++-- 6 files changed, 130 insertions(+), 8 deletions(-) create mode 100644 src/app/crypto/engineCrypto/backupKeyFrom4S.test.ts diff --git a/src/app/components/DeviceVerification.tsx b/src/app/components/DeviceVerification.tsx index f2c582566..f44c640a4 100644 --- a/src/app/components/DeviceVerification.tsx +++ b/src/app/components/DeviceVerification.tsx @@ -173,7 +173,7 @@ function SasVerification({ verifier, onCancel }: SasVerificationProps) { useVerifierCancel(verifier, onCancel); useEffect(() => { - verifier.verify(); + verifier.verify().catch(() => undefined); }, [verifier]); if (sasData) { diff --git a/src/app/crypto/engineCrypto/EngineCrypto.ts b/src/app/crypto/engineCrypto/EngineCrypto.ts index 0014faaac..699c8c6af 100644 --- a/src/app/crypto/engineCrypto/EngineCrypto.ts +++ b/src/app/crypto/engineCrypto/EngineCrypto.ts @@ -28,7 +28,10 @@ import type { RoomMessageEventContent } from 'matrix-js-sdk/lib/types'; import { encodeUri } from 'matrix-js-sdk/lib/utils'; import { TypedEventEmitter } from 'matrix-js-sdk/lib/models/typed-event-emitter'; import { CryptoEvent, DeviceIsolationModeKind } from 'matrix-js-sdk/lib/crypto-api'; -import { DecryptionFailureCode } from 'matrix-js-sdk/lib/crypto-api'; +import { + DecryptionFailureCode, + DecryptionKeyDoesNotMatchError, +} from 'matrix-js-sdk/lib/crypto-api'; import { DecryptionError } from 'matrix-js-sdk/lib/common-crypto/CryptoBackend'; import type { CryptoEventHandlerMap } from 'matrix-js-sdk/lib/crypto-api/CryptoEventHandlerMap'; import { createDebugLogger } from '$utils/debugLogger'; @@ -1894,9 +1897,29 @@ export class EngineCrypto const backupInfo = await this.#requestKeyBackupVersion(); if (!backupInfo?.version) throw new Error('No key backup version to attach the key to'); + if (!EngineCrypto.#keyMatchesBackup(encoded, backupInfo)) { + throw new DecryptionKeyDoesNotMatchError( + 'loadSessionBackupPrivateKeyFromSecretStorage: decryption key does not match backup info' + ); + } + await this.storeSessionBackupPrivateKey(decodeBase64(encoded), backupInfo.version); } + static #keyMatchesBackup(encoded: string, backupInfo: KeyBackupInfo): boolean { + const publicKey = (backupInfo.auth_data as { public_key?: string } | undefined)?.public_key; + try { + const key = RustSdkCryptoJs.BackupDecryptionKey.fromBase64(encoded); + try { + return key.megolmV1PublicKey.publicKeyBase64 === publicKey; + } finally { + key.free(); + } + } catch { + return false; + } + } + async getActiveSessionBackupVersion(): Promise { return (await this.#call('backupVersion')) as string | null; } diff --git a/src/app/crypto/engineCrypto/backupKeyFrom4S.test.ts b/src/app/crypto/engineCrypto/backupKeyFrom4S.test.ts new file mode 100644 index 000000000..35853b72c --- /dev/null +++ b/src/app/crypto/engineCrypto/backupKeyFrom4S.test.ts @@ -0,0 +1,50 @@ +import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import * as RustSdkCryptoJs from '@matrix-org/matrix-sdk-crypto-wasm'; +import { DecryptionKeyDoesNotMatchError } from 'matrix-js-sdk/lib/crypto-api'; +import type { MatrixClient } from '$types/matrix-sdk'; +import { engineInvoke } from '../olmMachine/engineInvoke'; +import { EngineCrypto } from './EngineCrypto'; + +vi.mock('../olmMachine/engineInvoke', () => ({ engineInvoke: vi.fn() })); + +const mockInvoke = vi.mocked(engineInvoke); + +const clientWith = (encoded: string, publicKey: string) => + ({ + secretStorage: { get: async () => encoded }, + http: { + authedRequest: vi.fn(async () => ({ + version: '3', + algorithm: 'm.megolm_backup.v1.curve25519-aes-sha2', + auth_data: { public_key: publicKey }, + })), + }, + }) as unknown as MatrixClient; + +describe('loadSessionBackupPrivateKeyFromSecretStorage', () => { + beforeAll(() => RustSdkCryptoJs.initAsync()); + beforeEach(() => mockInvoke.mockReset().mockResolvedValue(null)); + + it('rejects a stored key that does not match the server backup', async () => { + const stored = RustSdkCryptoJs.BackupDecryptionKey.createRandomKey(); + const other = RustSdkCryptoJs.BackupDecryptionKey.createRandomKey(); + const mx = clientWith(stored.toBase64(), other.megolmV1PublicKey.publicKeyBase64); + + const crypto = new EngineCrypto(mx, { userId: '@me:e.org', deviceId: 'D' }); + + await expect(crypto.loadSessionBackupPrivateKeyFromSecretStorage()).rejects.toBeInstanceOf( + DecryptionKeyDoesNotMatchError + ); + expect(mockInvoke.mock.calls.some(([, m]) => m === 'saveBackupDecryptionKey')).toBe(false); + }); + + it('accepts a stored key that matches the server backup', async () => { + const stored = RustSdkCryptoJs.BackupDecryptionKey.createRandomKey(); + const mx = clientWith(stored.toBase64(), stored.megolmV1PublicKey.publicKeyBase64); + + const crypto = new EngineCrypto(mx, { userId: '@me:e.org', deviceId: 'D' }); + + await expect(crypto.loadSessionBackupPrivateKeyFromSecretStorage()).resolves.toBeUndefined(); + expect(mockInvoke.mock.calls.some(([, m]) => m === 'saveBackupDecryptionKey')).toBe(true); + }); +}); diff --git a/src/app/crypto/verification/request.ts b/src/app/crypto/verification/request.ts index b8a071adb..cd4521790 100644 --- a/src/app/crypto/verification/request.ts +++ b/src/app/crypto/verification/request.ts @@ -247,7 +247,11 @@ export class EngineVerificationRequest await this.#call('verificationRequest.startSas', this.#flow); await this.refresh(); - if (!this.#verifier) throw new Error(`Starting ${method} produced no verifier`); + if (!this.#verifier) { + throw new Error( + `Could not start ${method}: the other device is no longer available for verification` + ); + } return this.#verifier; } diff --git a/src/app/crypto/verification/verifier.test.ts b/src/app/crypto/verification/verifier.test.ts index 6d1e0abe3..486e8ad72 100644 --- a/src/app/crypto/verification/verifier.test.ts +++ b/src/app/crypto/verification/verifier.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from 'vitest'; import { VerificationPhase, VerifierEvent, + type MatrixEvent, type ShowQrCodeCallbacks, type ShowSasCallbacks, } from '$types/matrix-sdk'; @@ -10,6 +11,43 @@ import { EngineQrVerifier, EngineSasVerifier } from './verifier'; const flow = { userId: '@them:e.org', flowId: '$f' }; describe('EngineSasVerifier', () => { + it('settles the flow when the user says the codes do not match', async () => { + const call = vi.fn<(method: string, args: unknown) => Promise>(async () => null); + const verifier = new EngineSasVerifier(call, flow, {}, '@them:e.org'); + const cancelled = vi.fn<(error: Error | MatrixEvent) => void>(); + verifier.on(VerifierEvent.Cancel, cancelled); + const completion = verifier.verify(); + + verifier.onChange({ + emoji: [{ symbol: '🐶', description: 'Dog' }], + decimals: [1, 2, 3], + }); + verifier.getShowSasCallbacks()?.mismatch(); + + await expect(completion).rejects.toThrow('The codes did not match'); + expect(cancelled).toHaveBeenCalledOnce(); + expect(call).toHaveBeenCalledWith( + 'sas.cancel', + expect.objectContaining({ code: 'm.mismatched_sas' }) + ); + }); + + it('does not send a second cancel when the user double taps', async () => { + const call = vi.fn<(method: string, args: unknown) => Promise>(async () => null); + const verifier = new EngineSasVerifier(call, flow, {}, '@them:e.org'); + verifier.verify().catch(() => undefined); + + verifier.onChange({ + emoji: [{ symbol: '🐶', description: 'Dog' }], + decimals: [1, 2, 3], + }); + const callbacks = verifier.getShowSasCallbacks(); + callbacks?.cancel(); + callbacks?.cancel(); + + expect(call.mock.calls.filter(([method]) => method === 'sas.cancel')).toHaveLength(1); + }); + // Reading the digits straight after accepting yields nothing: they arrive later. it('emits ShowSas when the digits arrive, not when accept is sent', async () => { const call = vi.fn<() => Promise>(async () => null); diff --git a/src/app/crypto/verification/verifier.ts b/src/app/crypto/verification/verifier.ts index fecdbb603..84763e7fc 100644 --- a/src/app/crypto/verification/verifier.ts +++ b/src/app/crypto/verification/verifier.ts @@ -107,8 +107,17 @@ abstract class EngineVerifier abstract verify(): Promise; cancel(error: Error): void { + this.finishCancelled(this.flow, error); + } + + protected cancelWithCode(code: string, error: Error): void { + this.finishCancelled({ ...this.flow, code }, error); + } + + private finishCancelled(flow: Record, error: Error): void { + if (this.hasBeenCancelled) return; this.markCancelled(); - void this.call(this.cancelMethod, this.flow); + void this.call(this.cancelMethod, flow); this.completion.reject(error); this.emit(VerifierEvent.Cancel, error); } @@ -169,12 +178,10 @@ export class EngineSasVerifier extends EngineVerifier { await this.call('sas.confirm', this.flow); }, mismatch: () => { - this.markCancelled(); - void this.call('sas.cancel', { ...this.flow, code: 'm.mismatched_sas' }); + this.cancelWithCode('m.mismatched_sas', new Error('The codes did not match')); }, cancel: () => { - this.markCancelled(); - void this.call('sas.cancel', { ...this.flow, code: 'm.user' }); + this.cancelWithCode('m.user', new Error('Verification cancelled')); }, }; } From f411d96fe0ddbd7f7e4d205ff8f0243c65924d7e Mon Sep 17 00:00:00 2001 From: Erwan Leboucher Date: Sun, 30 Aug 2026 20:59:33 +0200 Subject: [PATCH 08/10] test(crypto): port upstream's request, encryption and cross-signing cases --- src-tauri/src/matrix_crypto/devices.rs | 34 ++++++++- .../crypto/engineCrypto/claimChain.test.ts | 74 +++++++++++++++++++ .../engineCrypto/crossSigningKeys.test.ts | 70 ++++++++++++++++++ .../engineCrypto/encryptionOrder.test.ts | 45 ++++++++++- .../crypto/engineCrypto/outgoingRetry.test.ts | 64 ++++++++++++++-- src/app/crypto/verification/request.test.ts | 44 +++++++++++ src/app/crypto/verification/request.ts | 10 ++- src/app/crypto/verification/state.ts | 2 +- src/app/crypto/verification/verifier.ts | 2 + 9 files changed, 333 insertions(+), 12 deletions(-) create mode 100644 src/app/crypto/engineCrypto/claimChain.test.ts create mode 100644 src/app/crypto/engineCrypto/crossSigningKeys.test.ts diff --git a/src-tauri/src/matrix_crypto/devices.rs b/src-tauri/src/matrix_crypto/devices.rs index afa75758c..d78986e5a 100644 --- a/src-tauri/src/matrix_crypto/devices.rs +++ b/src-tauri/src/matrix_crypto/devices.rs @@ -330,10 +330,42 @@ pub async fn invoke( #[cfg(test)] mod tests { - use super::timeout; + use super::{invoke, timeout}; + use matrix_sdk_crypto::OlmMachine; use serde_json::json; use std::time::Duration; + async fn machine() -> OlmMachine { + let user: &matrix_sdk::ruma::UserId = "@me:example.org".try_into().unwrap(); + OlmMachine::new(user, "MYDEVICE".into()).await + } + + /// Returning success for a device we do not know lets the UI report that a device was + /// verified or cross-signed when nothing was signed at all. + #[tokio::test] + async fn verifying_an_unknown_device_is_an_error() { + let machine = machine().await; + let args = json!({ "userId": "@me:example.org", "deviceId": "NOSUCHDEVICE" }); + + let result = invoke(&machine, "device.verify", &args).await; + + assert!(matches!(result, Some(Err(_)))); + } + + #[tokio::test] + async fn trusting_an_unknown_device_is_an_error() { + let machine = machine().await; + let args = json!({ + "userId": "@me:example.org", + "deviceId": "NOSUCHDEVICE", + "trustState": 1, + }); + + let result = invoke(&machine, "device.setLocalTrust", &args).await; + + assert!(matches!(result, Some(Err(_)))); + } + /// `timeoutSecs` arrives verbatim from the webview, and `Duration::from_secs_f64` /// panics on negative, non-finite or overflowing values. #[test] diff --git a/src/app/crypto/engineCrypto/claimChain.test.ts b/src/app/crypto/engineCrypto/claimChain.test.ts new file mode 100644 index 000000000..4bb4931ae --- /dev/null +++ b/src/app/crypto/engineCrypto/claimChain.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it, vi } from 'vitest'; +import type { MatrixClient, MatrixEvent, Room } from '$types/matrix-sdk'; +import { engineInvoke } from '../olmMachine/engineInvoke'; +import { EngineCrypto } from './EngineCrypto'; + +vi.mock('../olmMachine/engineInvoke', () => ({ engineInvoke: vi.fn() })); + +const mockInvoke = vi.mocked(engineInvoke); + +const room = (roomId: string) => + ({ + roomId, + getEncryptionTargetMembers: async () => [{ userId: '@them:e.org' }], + getHistoryVisibility: () => 'shared', + getBlacklistUnverifiedDevices: () => false, + currentState: { getStateEvents: () => null }, + }) as unknown as Room; + +const event = () => + ({ + getType: () => 'm.room.message', + getContent: () => ({}), + makeEncrypted: vi.fn(), + getTxnId: () => 't', + }) as unknown as MatrixEvent; + +const client = () => + ({ http: { authedRequest: vi.fn(async () => '{}') } }) as unknown as MatrixClient; + +describe('key claim serialisation', () => { + it('never claims keys for two rooms at the same time', async () => { + const marks: string[] = []; + let inFlight = 0; + + mockInvoke.mockImplementation(async (_identity, method) => { + if (method === 'getMissingSessions') { + inFlight += 1; + marks.push(`enter${inFlight}`); + await new Promise((resolve) => { + setTimeout(resolve, 0); + }); + inFlight -= 1; + marks.push('exit'); + return null; + } + if (method === 'encryptRoomEvent') return '{}'; + if (method === 'identityKeys') return { ed25519: 'e', curve25519: 'c' }; + return null; + }); + + const crypto = new EngineCrypto(client(), { userId: '@me:e.org', deviceId: 'D' }); + + await Promise.all([ + crypto.encryptEvent(event(), room('!a:e.org')), + crypto.encryptEvent(event(), room('!b:e.org')), + ]); + + expect(marks).toEqual(['enter1', 'exit', 'enter1', 'exit']); + }); + + it('claims keys for every encryption target member', async () => { + mockInvoke.mockImplementation(async (_identity, method) => { + if (method === 'encryptRoomEvent') return '{}'; + if (method === 'identityKeys') return { ed25519: 'e', curve25519: 'c' }; + return null; + }); + + const crypto = new EngineCrypto(client(), { userId: '@me:e.org', deviceId: 'D' }); + await crypto.encryptEvent(event(), room('!a:e.org')); + + const args = mockInvoke.mock.calls.find(([, method]) => method === 'getMissingSessions')?.[2]; + expect(args).toEqual({ users: ['@them:e.org'] }); + }); +}); diff --git a/src/app/crypto/engineCrypto/crossSigningKeys.test.ts b/src/app/crypto/engineCrypto/crossSigningKeys.test.ts new file mode 100644 index 000000000..058ef55ea --- /dev/null +++ b/src/app/crypto/engineCrypto/crossSigningKeys.test.ts @@ -0,0 +1,70 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { MatrixClient } from '$types/matrix-sdk'; +import { engineInvoke } from '../olmMachine/engineInvoke'; +import { EngineCrypto } from './EngineCrypto'; + +vi.mock('../olmMachine/engineInvoke', () => ({ engineInvoke: vi.fn() })); + +const mockInvoke = vi.mocked(engineInvoke); + +const invoked = (method: string) => mockInvoke.mock.calls.filter(([, called]) => called === method); + +const crypto = () => + new EngineCrypto( + { http: { authedRequest: vi.fn(async () => '{}') } } as unknown as MatrixClient, + { + userId: '@me:e.org', + deviceId: 'D', + } + ); + +describe('userHasCrossSigningKeys', () => { + beforeEach(() => mockInvoke.mockReset()); + + // Answering false from a stale store makes callers rotate our cross-signing keys and + // invalidate every existing verification, so our own identity is always refreshed. + it('refreshes keys/query before answering for our own user', async () => { + mockInvoke.mockImplementation(async (_identity, method) => { + if (method === 'queryKeysForUsers') return { id: 'q1', type: 1, body: '{}' }; + if (method === 'getIdentity') return { isVerified: false }; + return null; + }); + + await expect(crypto().userHasCrossSigningKeys()).resolves.toBe(true); + + const order = mockInvoke.mock.calls.map(([, method]) => method); + expect(order.indexOf('queryKeysForUsers')).toBeLessThan(order.indexOf('getIdentity')); + expect(invoked('queryKeysForUsers')[0]?.[2]).toMatchObject({ users: ['@me:e.org'] }); + }); + + it('reports no identity for our own user only after a successful refresh', async () => { + mockInvoke.mockImplementation(async (_identity, method) => { + if (method === 'queryKeysForUsers') return null; + if (method === 'getIdentity') return null; + return null; + }); + + await expect(crypto().userHasCrossSigningKeys()).resolves.toBe(false); + expect(invoked('queryKeysForUsers')).toHaveLength(1); + }); + + it('does not query for another user unless asked to', async () => { + mockInvoke.mockImplementation(async (_identity, method) => + method === 'getIdentity' ? null : null + ); + + await expect(crypto().userHasCrossSigningKeys('@them:e.org')).resolves.toBe(false); + expect(invoked('queryKeysForUsers')).toHaveLength(0); + }); + + it('queries for another user when downloadUncached is set', async () => { + mockInvoke.mockImplementation(async (_identity, method) => { + if (method === 'queryKeysForUsers') return { id: 'q1', type: 1, body: '{}' }; + if (method === 'getIdentity') return { isVerified: false }; + return null; + }); + + await expect(crypto().userHasCrossSigningKeys('@them:e.org', true)).resolves.toBe(true); + expect(invoked('queryKeysForUsers')[0]?.[2]).toMatchObject({ users: ['@them:e.org'] }); + }); +}); diff --git a/src/app/crypto/engineCrypto/encryptionOrder.test.ts b/src/app/crypto/engineCrypto/encryptionOrder.test.ts index 07d0e3951..5b56345d5 100644 --- a/src/app/crypto/engineCrypto/encryptionOrder.test.ts +++ b/src/app/crypto/engineCrypto/encryptionOrder.test.ts @@ -7,10 +7,13 @@ vi.mock('../olmMachine/engineInvoke', () => ({ engineInvoke: vi.fn() })); const mockInvoke = vi.mocked(engineInvoke); -const room = (roomId: string) => +const room = (roomId: string, onMembers?: () => void) => ({ roomId, - getEncryptionTargetMembers: async () => [{ userId: '@me:e.org' }], + getEncryptionTargetMembers: async () => { + onMembers?.(); + return [{ userId: '@me:e.org' }]; + }, getHistoryVisibility: () => 'shared', getBlacklistUnverifiedDevices: () => false, currentState: { getStateEvents: () => null }, @@ -25,6 +28,44 @@ const event = () => }) as unknown as MatrixEvent; describe('encryptEvent ordering', () => { + // element-web#26684: an edit must not overtake the message it edits. Looking members up + // outside the per-room chain reintroduces that race while keeping mutual exclusion. + it('does not look up members for the next event until the previous one is encrypted', async () => { + let lookups = 0; + let release: (() => void) | undefined; + const held = new Promise((resolve) => { + release = resolve; + }); + + mockInvoke.mockImplementation(async (_identity, method) => { + if (method === 'encryptRoomEvent') { + await held; + return '{}'; + } + if (method === 'identityKeys') return { ed25519: 'e', curve25519: 'c' }; + return null; + }); + + const mx = { http: { authedRequest: vi.fn() } } as unknown as MatrixClient; + const crypto = new EngineCrypto(mx, { userId: '@me:e.org', deviceId: 'D' }); + const target = room('!r:e.org', () => { + lookups += 1; + }); + + const both = Promise.all([ + crypto.encryptEvent(event(), target), + crypto.encryptEvent(event(), target), + ]); + + await Promise.resolve(); + await Promise.resolve(); + expect(lookups).toBe(1); + + release?.(); + await both; + expect(lookups).toBe(2); + }); + it('never runs two encryptions for a room at the same time', async () => { const marks: string[] = []; let inFlight = 0; diff --git a/src/app/crypto/engineCrypto/outgoingRetry.test.ts b/src/app/crypto/engineCrypto/outgoingRetry.test.ts index 370900c2e..a7bee5305 100644 --- a/src/app/crypto/engineCrypto/outgoingRetry.test.ts +++ b/src/app/crypto/engineCrypto/outgoingRetry.test.ts @@ -1,5 +1,6 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { Method } from 'matrix-js-sdk/lib/http-api'; +import { ConnectionError, MatrixError, Method } from 'matrix-js-sdk/lib/http-api'; +import { sleep } from 'matrix-js-sdk/lib/utils'; import type { MatrixClient } from '$types/matrix-sdk'; import { RequestType, sendOutgoingRequest } from './outgoing'; @@ -8,11 +9,14 @@ vi.mock('matrix-js-sdk/lib/utils', async (importOriginal) => ({ sleep: vi.fn(async () => undefined), })); -const rateLimited = () => - Object.assign(new Error('rate limited'), { - httpStatus: 429, - data: { errcode: 'M_LIMIT_EXCEEDED', retry_after_ms: 10 }, - }); +const rateLimited = (retryAfterMs = 10) => + new MatrixError( + { errcode: 'M_LIMIT_EXCEEDED', error: 'Too many requests', retry_after_ms: retryAfterMs }, + 429 + ); + +const clientWith = (authedRequest: unknown) => + ({ http: { authedRequest } }) as unknown as MatrixClient; const request = { id: 'r1', type: RequestType.KeysClaim, body: '{}' }; @@ -31,6 +35,54 @@ describe('sendOutgoingRequest', () => { expect(authedRequest.mock.calls[0]?.[0]).toBe(Method.Post); }); + it('waits as long as the server asks after a rate limit', async () => { + const authedRequest = vi + .fn<(...args: never[]) => Promise>() + .mockRejectedValueOnce(rateLimited(5000)) + .mockResolvedValueOnce('{}'); + + await sendOutgoingRequest(clientWith(authedRequest), request); + + expect(vi.mocked(sleep)).toHaveBeenCalledWith(5000); + }); + + it('gives up after five attempts on a persistent server error', async () => { + const authedRequest = vi + .fn<(...args: never[]) => Promise>() + .mockRejectedValue(new MatrixError({ errcode: 'M_UNKNOWN', error: 'boom' }, 500)); + + await expect(sendOutgoingRequest(clientWith(authedRequest), request)).rejects.toThrow(); + expect(authedRequest).toHaveBeenCalledTimes(5); + }); + + it('does not retry a request the server says is too large', async () => { + const authedRequest = vi + .fn<(...args: never[]) => Promise>() + .mockRejectedValue(new MatrixError({ errcode: 'M_TOO_LARGE', error: 'too big' }, 502)); + + await expect(sendOutgoingRequest(clientWith(authedRequest), request)).rejects.toThrow(); + expect(authedRequest).toHaveBeenCalledTimes(1); + }); + + it('does not retry a request we aborted ourselves', async () => { + const authedRequest = vi + .fn<(...args: never[]) => Promise>() + .mockRejectedValue(Object.assign(new Error('aborted'), { name: 'AbortError' })); + + await expect(sendOutgoingRequest(clientWith(authedRequest), request)).rejects.toThrow(); + expect(authedRequest).toHaveBeenCalledTimes(1); + }); + + it('retries after a connection error', async () => { + const authedRequest = vi + .fn<(...args: never[]) => Promise>() + .mockRejectedValueOnce(new ConnectionError('Failed to fetch')) + .mockResolvedValueOnce('{}'); + + await expect(sendOutgoingRequest(clientWith(authedRequest), request)).resolves.toBe('{}'); + expect(authedRequest).toHaveBeenCalledTimes(2); + }); + it('rethrows an error that is not worth retrying', async () => { const authedRequest = vi .fn<(...args: never[]) => Promise>() diff --git a/src/app/crypto/verification/request.test.ts b/src/app/crypto/verification/request.test.ts index ccb8ded90..80282a025 100644 --- a/src/app/crypto/verification/request.test.ts +++ b/src/app/crypto/verification/request.test.ts @@ -30,6 +30,50 @@ const state = (patch: Partial = {}): EngineVerification }); describe('EngineVerificationRequest', () => { + // Both sides press verify at the same moment; the loser's Sas is replaced by a fresh + // one that has not been accepted, and must be re-accepted or the flow hangs. + it('re-accepts when our SAS is replaced after losing the start tie-break', async () => { + const call = vi.fn<(m: string, a?: Record) => Promise>( + async () => null + ); + const started = state({ + phase: EnginePhase.Transitioned, + verification: { className: 'Sas', hasBeenAccepted: true }, + }); + const request = new EngineVerificationRequest(call, started); + call.mockClear(); + + request.apply( + state({ + phase: EnginePhase.Transitioned, + verification: { className: 'Sas', hasBeenAccepted: false }, + }) + ); + + expect(call.mock.calls.filter(([method]) => method === 'sas.accept')).toHaveLength(1); + }); + + it('does not re-accept while the same SAS stays accepted', async () => { + const call = vi.fn<(m: string, a?: Record) => Promise>( + async () => null + ); + const started = state({ + phase: EnginePhase.Transitioned, + verification: { className: 'Sas', hasBeenAccepted: true }, + }); + const request = new EngineVerificationRequest(call, started); + call.mockClear(); + + request.apply( + state({ + phase: EnginePhase.Transitioned, + verification: { className: 'Sas', hasBeenAccepted: true }, + }) + ); + + expect(call.mock.calls.filter(([method]) => method === 'sas.accept')).toHaveLength(0); + }); + it('accepts advertising every method we support, not the empty set the engine reports', async () => { const call = vi.fn<(m: string, a?: Record) => Promise>( async (method) => diff --git a/src/app/crypto/verification/request.ts b/src/app/crypto/verification/request.ts index cd4521790..70eb65d81 100644 --- a/src/app/crypto/verification/request.ts +++ b/src/app/crypto/verification/request.ts @@ -42,6 +42,8 @@ export class EngineVerificationRequest #declining = false; + #sasAccepted = false; + constructor(call: EngineCall, state: EngineVerificationState) { super(); this.#call = call; @@ -72,7 +74,11 @@ export class EngineVerificationRequest ? 'Qr' : undefined; - if (current !== wanted) { + const accepted = (verification as SasState).hasBeenAccepted === true; + const lostTieBreak = wanted === 'Sas' && current === 'Sas' && this.#sasAccepted && !accepted; + this.#sasAccepted = wanted === 'Sas' ? accepted : false; + + if (current !== wanted || lostTieBreak) { if (wanted === 'Sas') { this.#verifier = new EngineSasVerifier( this.#call, @@ -80,7 +86,7 @@ export class EngineVerificationRequest verification as SasState, this.#state.otherUserId ); - if (current !== undefined) void this.#reaccept(); + if (current !== undefined || lostTieBreak) void this.#reaccept(); } else if (wanted === 'Qr') { this.#verifier = new EngineQrVerifier( this.#call, diff --git a/src/app/crypto/verification/state.ts b/src/app/crypto/verification/state.ts index dadf181d2..57860499f 100644 --- a/src/app/crypto/verification/state.ts +++ b/src/app/crypto/verification/state.ts @@ -47,7 +47,7 @@ export type EngineVerificationState = { theirSupportedMethods?: number[] | null; ourSupportedMethods?: number[] | null; cancelInfo?: EngineCancelInfo | null; - verification?: { className?: string; isDone?: boolean } | null; + verification?: { className?: string; isDone?: boolean; hasBeenAccepted?: boolean } | null; }; export const methodFromCode = (code: number): string | undefined => METHOD_BY_CODE[code]; diff --git a/src/app/crypto/verification/verifier.ts b/src/app/crypto/verification/verifier.ts index 84763e7fc..6b951870f 100644 --- a/src/app/crypto/verification/verifier.ts +++ b/src/app/crypto/verification/verifier.ts @@ -13,6 +13,8 @@ export type EngineCall = (method: string, args?: Record) => Pro export type SasState = { className?: string; + weStarted?: boolean; + hasBeenAccepted?: boolean; canBePresented?: boolean; haveWeConfirmed?: boolean; isDone?: boolean; From 417c6587596a92511111c1988e8207527ec9cb6f Mon Sep 17 00:00:00 2001 From: Erwan Leboucher Date: Sun, 30 Aug 2026 21:11:16 +0200 Subject: [PATCH 09/10] fix(crypto): stop claiming the server has no fallback keys and honour the caller's trust requirement --- src-tauri/src/matrix_crypto/backup.rs | 109 ++++++++++++++++++ src-tauri/src/matrix_crypto/dispatch.rs | 11 +- src-tauri/src/matrix_crypto/wasm_enums.rs | 43 +++++++ src/app/crypto/engineCrypto/EngineCrypto.ts | 12 +- .../crypto/engineCrypto/engineShapes.test.ts | 23 +++- .../engineCrypto/secretStorageAccess.ts | 15 +++ src/app/crypto/verification/verifier.test.ts | 48 ++++++++ src/app/crypto/verification/verifier.ts | 14 +-- 8 files changed, 255 insertions(+), 20 deletions(-) create mode 100644 src/app/crypto/engineCrypto/secretStorageAccess.ts diff --git a/src-tauri/src/matrix_crypto/backup.rs b/src-tauri/src/matrix_crypto/backup.rs index ebff84879..52ec9ecd5 100644 --- a/src-tauri/src/matrix_crypto/backup.rs +++ b/src-tauri/src/matrix_crypto/backup.rs @@ -218,6 +218,7 @@ async fn handle(machine: &OlmMachine, method: &str, args: &Value) -> Result OlmMachine { + let user: &matrix_sdk::ruma::UserId = "@backup:example.org".try_into().unwrap(); + OlmMachine::new(user, "BACKUPDEV".into()).await + } + + /// EngineCrypto.ts reads these exact spellings; a rename here is invisible to the + /// compiler and silently breaks restore. + #[tokio::test] + async fn a_saved_decryption_key_reads_back_in_the_wasm_shape() { + let machine = machine().await; + let key = BackupDecryptionKey::new().to_base64(); + + let saved = invoke( + &machine, + "saveBackupDecryptionKey", + &json!({ "decryptionKey": key, "version": "7" }), + ) + .await + .unwrap() + .unwrap(); + assert_eq!(saved, Value::Null); + + let keys = invoke(&machine, "getBackupKeys", &json!({})) + .await + .unwrap() + .unwrap(); + assert_eq!(keys["className"], "BackupKeys"); + assert_eq!(keys["backupVersion"], "7"); + assert_eq!(keys["decryptionKeyBase64"], key); + } + + #[tokio::test] + async fn an_empty_store_reports_nulls_rather_than_erroring() { + let machine = machine().await; + + let keys = invoke(&machine, "getBackupKeys", &json!({})) + .await + .unwrap() + .unwrap(); + assert!(keys["backupVersion"].is_null()); + assert!(keys["decryptionKeyBase64"].is_null()); + + assert_eq!( + invoke(&machine, "isBackupEnabled", &json!({})) + .await + .unwrap() + .unwrap(), + json!(false) + ); + assert_eq!( + invoke(&machine, "backupVersion", &json!({})) + .await + .unwrap() + .unwrap(), + Value::Null + ); + } + + #[tokio::test] + async fn enabling_a_backup_makes_its_version_readable() { + let machine = machine().await; + let public = BackupDecryptionKey::new() + .megolm_v1_public_key() + .to_base64(); + + invoke( + &machine, + "enableBackupV1", + &json!({ "publicKeyBase64": public, "version": "3" }), + ) + .await + .unwrap() + .unwrap(); + + assert_eq!( + invoke(&machine, "isBackupEnabled", &json!({})) + .await + .unwrap() + .unwrap(), + json!(true) + ); + assert_eq!( + invoke(&machine, "backupVersion", &json!({})) + .await + .unwrap() + .unwrap(), + json!("3") + ); + } + + /// The key itself must never reach an error string or a log. + #[tokio::test] + async fn a_malformed_decryption_key_is_an_error_that_does_not_leak_it() { + let machine = machine().await; + + let result = invoke( + &machine, + "saveBackupDecryptionKey", + &json!({ "decryptionKey": "notBase64", "version": "1" }), + ) + .await; + + let Some(Err(message)) = result else { + panic!("expected an error"); + }; + assert!(!message.contains("notBase64"), "{message}"); + } + const VALID_CURVE_KEY: &str = "KyHFkVuB9MFbEkiCw+idNHKbiM8r3cWpNNPdyHkFeHY"; fn valid_session_key() -> String { diff --git a/src-tauri/src/matrix_crypto/dispatch.rs b/src-tauri/src/matrix_crypto/dispatch.rs index bb6e4a22c..6d6caf48e 100644 --- a/src-tauri/src/matrix_crypto/dispatch.rs +++ b/src-tauri/src/matrix_crypto/dispatch.rs @@ -21,7 +21,7 @@ use serde_json::{json, Value}; use matrix_sdk::deserialized_responses::{DeviceLinkProblem, VerificationLevel}; use matrix_sdk_crypto::MegolmError; -use super::args::{caller_decryption_settings, decryption_settings, room_id, str_arg}; +use super::args::{caller_decryption_settings, room_id, str_arg}; use super::requests::{mark_request_sent, outgoing_requests}; use super::wasm_enums::processed_to_device_event_type; @@ -269,7 +269,7 @@ pub async fn invoke(machine: &OlmMachine, method: &str, args: Value) -> Result = args + let fallback_keys: Option> = args .get("unusedFallbackKeys") .and_then(Value::as_array) .map(|keys| { @@ -277,8 +277,7 @@ pub async fn invoke(machine: &OlmMachine, method: &str, args: Value) -> Result Result u8 { _ => 2, } } + +#[cfg(test)] +mod tests { + use super::*; + + /// The webview sends and reads these as bare numbers from matrix-sdk-crypto-wasm, + /// which is versioned separately from this crate, so nothing links the two at + /// compile time. Mirrors requests.test.js and encryption.test.ts upstream. + #[test] + fn request_type_codes_match_the_wasm_enum() { + assert_eq!(request_type::KEYS_UPLOAD, 0); + assert_eq!(request_type::KEYS_QUERY, 1); + assert_eq!(request_type::KEYS_CLAIM, 2); + assert_eq!(request_type::TO_DEVICE, 3); + assert_eq!(request_type::SIGNATURE_UPLOAD, 4); + assert_eq!(request_type::ROOM_MESSAGE, 5); + assert_eq!(request_type::KEYS_BACKUP, 6); + } + + #[test] + fn processed_to_device_event_codes_match_the_wasm_enum() { + assert_eq!(processed_to_device_event_type::DECRYPTED, 0); + assert_eq!(processed_to_device_event_type::UNABLE_TO_DECRYPT, 1); + assert_eq!(processed_to_device_event_type::PLAIN_TEXT, 2); + assert_eq!(processed_to_device_event_type::INVALID, 3); + } + + #[test] + fn encryption_algorithm_codes_match_the_wasm_enum() { + assert_eq!( + encryption_algorithm(&EventEncryptionAlgorithm::OlmV1Curve25519AesSha2), + 0 + ); + assert_eq!( + encryption_algorithm(&EventEncryptionAlgorithm::MegolmV1AesSha2), + 1 + ); + assert_eq!( + encryption_algorithm(&EventEncryptionAlgorithm::from("m.some.future.algorithm")), + 2 + ); + } +} diff --git a/src/app/crypto/engineCrypto/EngineCrypto.ts b/src/app/crypto/engineCrypto/EngineCrypto.ts index 699c8c6af..0a96b1f15 100644 --- a/src/app/crypto/engineCrypto/EngineCrypto.ts +++ b/src/app/crypto/engineCrypto/EngineCrypto.ts @@ -33,6 +33,7 @@ import { DecryptionKeyDoesNotMatchError, } from 'matrix-js-sdk/lib/crypto-api'; import { DecryptionError } from 'matrix-js-sdk/lib/common-crypto/CryptoBackend'; +import { secretStorageCanAccessSecrets } from './secretStorageAccess'; import type { CryptoEventHandlerMap } from 'matrix-js-sdk/lib/crypto-api/CryptoEventHandlerMap'; import { createDebugLogger } from '$utils/debugLogger'; import { EngineVerificationRequest } from '../verification/request'; @@ -1678,7 +1679,8 @@ export class EngineCrypto const entries = await Promise.all( names.map( - async (name) => [name, Boolean(await this.#mx.secretStorage.isStored(name))] as const + async (name) => + [name, await secretStorageCanAccessSecrets(this.#mx.secretStorage, [name])] as const ) ); const secretStorageKeyValidityMap = Object.fromEntries(entries); @@ -1721,13 +1723,13 @@ export class EngineCrypto hasSelfSigning: boolean; hasUserSigning: boolean; }; - const inStorage = await Promise.all( - SECRETS_IN_STORAGE.map(async (name) => Boolean(await this.#mx.secretStorage.isStored(name))) - ); + const inStorage = await secretStorageCanAccessSecrets(this.#mx.secretStorage, [ + ...SECRETS_IN_STORAGE, + ]); return { publicKeysOnDevice: status.hasMaster && status.hasSelfSigning && status.hasUserSigning, - privateKeysInSecretStorage: inStorage.every(Boolean), + privateKeysInSecretStorage: inStorage, privateKeysCachedLocally: { masterKey: status.hasMaster, selfSigningKey: status.hasSelfSigning, diff --git a/src/app/crypto/engineCrypto/engineShapes.test.ts b/src/app/crypto/engineCrypto/engineShapes.test.ts index 06cb2466c..987eb17dd 100644 --- a/src/app/crypto/engineCrypto/engineShapes.test.ts +++ b/src/app/crypto/engineCrypto/engineShapes.test.ts @@ -9,10 +9,15 @@ vi.mock('../olmMachine/engineInvoke', () => ({ const mockInvoke = vi.mocked(engineInvoke); -const crypto = (stored: string[] = []) => +const DEFAULT_KEY_ID = 'default-key'; + +const crypto = (stored: string[] = [], keyId: string = DEFAULT_KEY_ID) => new EngineCrypto( { - secretStorage: { isStored: async (name: string) => (stored.includes(name) ? {} : null) }, + secretStorage: { + getDefaultKeyId: async () => DEFAULT_KEY_ID, + isStored: async (name: string) => (stored.includes(name) ? { [keyId]: {} } : null), + }, } as unknown as MatrixClient, { userId: '@me:example.org', deviceId: 'DEVICE' } ); @@ -131,4 +136,18 @@ describe('engine payload shapes', () => { crypto(CROSS_SIGNING_SECRETS.slice(0, 2)).getCrossSigningStatus() ).resolves.toMatchObject({ privateKeysInSecretStorage: false }); }); + + // A secret left behind under a rotated-away 4S key is not recoverable, so reporting it + // as held would tell the user their keys are safe when they are not. + it('does not count secrets stored under a key that is no longer the default', async () => { + mockInvoke.mockResolvedValue({ + hasMaster: false, + hasSelfSigning: false, + hasUserSigning: false, + }); + + await expect( + crypto(CROSS_SIGNING_SECRETS, 'rotated-away-key').getCrossSigningStatus() + ).resolves.toMatchObject({ privateKeysInSecretStorage: false }); + }); }); diff --git a/src/app/crypto/engineCrypto/secretStorageAccess.ts b/src/app/crypto/engineCrypto/secretStorageAccess.ts new file mode 100644 index 000000000..9ce028581 --- /dev/null +++ b/src/app/crypto/engineCrypto/secretStorageAccess.ts @@ -0,0 +1,15 @@ +import type { SecretStorageKey, ServerSideSecretStorage } from 'matrix-js-sdk/lib/secret-storage'; + +export const secretStorageCanAccessSecrets = async ( + secretStorage: ServerSideSecretStorage, + secretNames: SecretStorageKey[] +): Promise => { + const defaultKeyId = await secretStorage.getDefaultKeyId(); + if (!defaultKeyId) return false; + + const stored = await Promise.all( + secretNames.map(async (name) => (await secretStorage.isStored(name)) ?? {}) + ); + + return stored.every((record) => defaultKeyId in record); +}; diff --git a/src/app/crypto/verification/verifier.test.ts b/src/app/crypto/verification/verifier.test.ts index 486e8ad72..b661e8284 100644 --- a/src/app/crypto/verification/verifier.test.ts +++ b/src/app/crypto/verification/verifier.test.ts @@ -11,6 +11,40 @@ import { EngineQrVerifier, EngineSasVerifier } from './verifier'; const flow = { userId: '@them:e.org', flowId: '$f' }; describe('EngineSasVerifier', () => { + it('emits Cancel when the other side cancels', async () => { + const call = vi.fn<(m: string, a?: Record) => Promise>( + async () => null + ); + const verifier = new EngineSasVerifier(call, flow, {}, '@them:e.org'); + const cancelled = vi.fn<(error: Error | MatrixEvent) => void>(); + verifier.on(VerifierEvent.Cancel, cancelled); + const verifying = verifier.verify(); + + verifier.onChange({ isCancelled: true }); + + await expect(verifying).rejects.toThrow('Verification cancelled'); + expect(cancelled).toHaveBeenCalledOnce(); + expect(call.mock.calls.filter(([method]) => method === 'sas.cancel')).toHaveLength(0); + }); + + it('does not cancel twice when the engine echoes our own mismatch', async () => { + const call = vi.fn<(m: string, a?: Record) => Promise>( + async () => null + ); + const verifier = new EngineSasVerifier(call, flow, {}, '@them:e.org'); + const cancelled = vi.fn<(error: Error | MatrixEvent) => void>(); + verifier.on(VerifierEvent.Cancel, cancelled); + const verifying = verifier.verify(); + + verifier.onChange({ decimals: [1, 2, 3] }); + verifier.getShowSasCallbacks()?.mismatch(); + verifier.onChange({ isCancelled: true }); + + await expect(verifying).rejects.toThrow('The codes did not match'); + expect(cancelled).toHaveBeenCalledOnce(); + expect(call.mock.calls.filter(([method]) => method === 'sas.cancel')).toHaveLength(1); + }); + it('settles the flow when the user says the codes do not match', async () => { const call = vi.fn<(method: string, args: unknown) => Promise>(async () => null); const verifier = new EngineSasVerifier(call, flow, {}, '@them:e.org'); @@ -139,6 +173,20 @@ describe('EngineSasVerifier', () => { }); describe('EngineQrVerifier', () => { + it('settles the flow when the user declines the reciprocated code', async () => { + const call = vi.fn<(m: string, a?: Record) => Promise>( + async () => null + ); + const verifier = new EngineQrVerifier(call, flow, {}, '@them:e.org'); + const verifying = verifier.verify(); + + verifier.onChange({ hasBeenScanned: true }); + verifier.getReciprocateQrCodeCallbacks()?.cancel(); + + await expect(verifying).rejects.toThrow('Verification cancelled'); + expect(call).toHaveBeenCalledWith('qr.cancel', expect.objectContaining({ code: 'm.user' })); + }); + it('offers reciprocate callbacks only once our code has been scanned', () => { const call = vi.fn<() => Promise>(async () => null); const verifier = new EngineQrVerifier(call, flow, {}, '@them:e.org'); diff --git a/src/app/crypto/verification/verifier.ts b/src/app/crypto/verification/verifier.ts index 6b951870f..176f2599a 100644 --- a/src/app/crypto/verification/verifier.ts +++ b/src/app/crypto/verification/verifier.ts @@ -98,8 +98,11 @@ abstract class EngineVerifier this.completion.resolve(); return; } + if (this.hasBeenCancelled) return; this.markCancelled(); - this.completion.reject(new Error('Verification cancelled')); + const error = new Error('Verification cancelled'); + this.completion.reject(error); + this.emit(VerifierEvent.Cancel, error); } abstract onChange(state: TState): void; @@ -157,8 +160,7 @@ export class EngineSasVerifier extends EngineVerifier { this.state = state; if (state.isCancelled) { - this.markCancelled(); - this.completion.reject(new Error('Verification cancelled')); + this.settle(false); return; } @@ -227,8 +229,7 @@ export class EngineQrVerifier extends EngineVerifier { this.state = state; if (state.isCancelled) { - this.markCancelled(); - this.completion.reject(new Error('Verification cancelled')); + this.settle(false); return; } @@ -238,8 +239,7 @@ export class EngineQrVerifier extends EngineVerifier { void this.call('qr.confirm', this.flow); }, cancel: () => { - this.markCancelled(); - void this.call('qr.cancel', { ...this.flow, code: 'm.user' }); + this.cancelWithCode('m.user', new Error('Verification cancelled')); }, }; this.emit(VerifierEvent.ShowReciprocateQr, this.#callbacks); From 6c0e899826e575aca402e208786d53e172b29917 Mon Sep 17 00:00:00 2001 From: Erwan Leboucher Date: Sun, 30 Aug 2026 21:21:44 +0200 Subject: [PATCH 10/10] feat(crypto): fetch missing session keys from backup and ask other devices for secrets --- src-tauri/src/matrix_crypto/backup.rs | 3 - src-tauri/src/matrix_crypto/cross_signing.rs | 10 ++ src-tauri/src/matrix_crypto/devices.rs | 4 - src-tauri/src/matrix_crypto/mod.rs | 5 - src-tauri/src/matrix_crypto/rooms.rs | 4 - src-tauri/src/matrix_crypto/wasm_enums.rs | 3 - src/app/crypto/engineCrypto/EngineCrypto.ts | 42 ++++++ .../engineCrypto/backupKeyFrom4S.test.ts | 6 +- .../crypto/engineCrypto/claimChain.test.ts | 10 +- .../engineCrypto/crossSigningKeys.test.ts | 10 +- .../engineCrypto/encryptionOrder.test.ts | 12 +- .../crypto/engineCrypto/engineShapes.test.ts | 2 - .../crypto/engineCrypto/outgoingRetry.test.ts | 17 ++- .../perSessionBackupDownload.test.ts | 112 ++++++++++++++++ .../engineCrypto/perSessionBackupDownload.ts | 124 ++++++++++++++++++ src/app/crypto/install.ts | 4 + src/app/crypto/verification/request.test.ts | 2 - 17 files changed, 327 insertions(+), 43 deletions(-) create mode 100644 src/app/crypto/engineCrypto/perSessionBackupDownload.test.ts create mode 100644 src/app/crypto/engineCrypto/perSessionBackupDownload.ts diff --git a/src-tauri/src/matrix_crypto/backup.rs b/src-tauri/src/matrix_crypto/backup.rs index 52ec9ecd5..ae7c74681 100644 --- a/src-tauri/src/matrix_crypto/backup.rs +++ b/src-tauri/src/matrix_crypto/backup.rs @@ -277,8 +277,6 @@ mod tests { OlmMachine::new(user, "BACKUPDEV".into()).await } - /// EngineCrypto.ts reads these exact spellings; a rename here is invisible to the - /// compiler and silently breaks restore. #[tokio::test] async fn a_saved_decryption_key_reads_back_in_the_wasm_shape() { let machine = machine().await; @@ -362,7 +360,6 @@ mod tests { ); } - /// The key itself must never reach an error string or a log. #[tokio::test] async fn a_malformed_decryption_key_is_an_error_that_does_not_leak_it() { let machine = machine().await; diff --git a/src-tauri/src/matrix_crypto/cross_signing.rs b/src-tauri/src/matrix_crypto/cross_signing.rs index 7cedff072..1e8ad36ea 100644 --- a/src-tauri/src/matrix_crypto/cross_signing.rs +++ b/src-tauri/src/matrix_crypto/cross_signing.rs @@ -39,11 +39,21 @@ pub async fn invoke( "importSecretsBundle" => import_secrets_bundle(machine, args).await, "pushSecretToVerifiedDevices" => push_secret(machine, args).await, + "requestMissingSecretsIfNeeded" => request_missing_secrets(machine).await, _ => return None, }) } +async fn request_missing_secrets(machine: &OlmMachine) -> Result { + let requested = machine + .query_missing_secrets_from_other_sessions() + .await + .map_err(|e| format!("requestMissingSecretsIfNeeded failed: {e}"))?; + + Ok(Value::Bool(requested)) +} + async fn bootstrap(machine: &OlmMachine, args: &Value) -> Result { let reset = args .get("reset") diff --git a/src-tauri/src/matrix_crypto/devices.rs b/src-tauri/src/matrix_crypto/devices.rs index d78986e5a..4d39f9ec2 100644 --- a/src-tauri/src/matrix_crypto/devices.rs +++ b/src-tauri/src/matrix_crypto/devices.rs @@ -340,8 +340,6 @@ mod tests { OlmMachine::new(user, "MYDEVICE".into()).await } - /// Returning success for a device we do not know lets the UI report that a device was - /// verified or cross-signed when nothing was signed at all. #[tokio::test] async fn verifying_an_unknown_device_is_an_error() { let machine = machine().await; @@ -366,8 +364,6 @@ mod tests { assert!(matches!(result, Some(Err(_)))); } - /// `timeoutSecs` arrives verbatim from the webview, and `Duration::from_secs_f64` - /// panics on negative, non-finite or overflowing values. #[test] fn an_unusable_timeout_is_ignored_rather_than_panicking() { for secs in [-1.0, f64::NAN, f64::INFINITY, 1e300] { diff --git a/src-tauri/src/matrix_crypto/mod.rs b/src-tauri/src/matrix_crypto/mod.rs index 6f0142910..a0bd8b3a7 100644 --- a/src-tauri/src/matrix_crypto/mod.rs +++ b/src-tauri/src/matrix_crypto/mod.rs @@ -324,8 +324,6 @@ mod tests { ); } - /// Enabling `automatic-room-key-forwarding` also enables OUTGOING room key - /// requests, which upstream turns off (element-web#26524). The two must stay paired. #[tokio::test] async fn outgoing_room_key_requests_stay_disabled() { let user: &matrix_sdk::ruma::UserId = "@gossip:example.org".try_into().unwrap(); @@ -344,9 +342,6 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } - /// A cold push and the webview can both be opening the same account. The push must - /// never deregister a machine it did not itself open, or the webview's crypto dies - /// for the rest of the session. #[tokio::test] async fn close_account_if_leaves_a_machine_it_does_not_own() { let user: &matrix_sdk::ruma::UserId = "@race:example.org".try_into().unwrap(); diff --git a/src-tauri/src/matrix_crypto/rooms.rs b/src-tauri/src/matrix_crypto/rooms.rs index 5e14b7ea2..5155eb751 100644 --- a/src-tauri/src/matrix_crypto/rooms.rs +++ b/src-tauri/src/matrix_crypto/rooms.rs @@ -454,10 +454,6 @@ mod tests { ); } - /// Membership comes from server state and is not validated by the SDK. A single - /// unparseable id used to abort the whole call, which left the user unable to send - /// any encrypted message in that room. `updateTrackedUsers` and `queryKeysForUsers` - /// already skip the same ids from the same array. #[test] fn an_unparseable_member_id_is_skipped_not_fatal() { let args = json!({ "users": ["@good:example.org", "@bad:under_score", "not-an-id", 7] }); diff --git a/src-tauri/src/matrix_crypto/wasm_enums.rs b/src-tauri/src/matrix_crypto/wasm_enums.rs index 8168367ce..03dd62e2e 100644 --- a/src-tauri/src/matrix_crypto/wasm_enums.rs +++ b/src-tauri/src/matrix_crypto/wasm_enums.rs @@ -34,9 +34,6 @@ pub fn encryption_algorithm(algorithm: &EventEncryptionAlgorithm) -> u8 { mod tests { use super::*; - /// The webview sends and reads these as bare numbers from matrix-sdk-crypto-wasm, - /// which is versioned separately from this crate, so nothing links the two at - /// compile time. Mirrors requests.test.js and encryption.test.ts upstream. #[test] fn request_type_codes_match_the_wasm_enum() { assert_eq!(request_type::KEYS_UPLOAD, 0); diff --git a/src/app/crypto/engineCrypto/EngineCrypto.ts b/src/app/crypto/engineCrypto/EngineCrypto.ts index 0a96b1f15..05ff58a86 100644 --- a/src/app/crypto/engineCrypto/EngineCrypto.ts +++ b/src/app/crypto/engineCrypto/EngineCrypto.ts @@ -34,6 +34,7 @@ import { } from 'matrix-js-sdk/lib/crypto-api'; import { DecryptionError } from 'matrix-js-sdk/lib/common-crypto/CryptoBackend'; import { secretStorageCanAccessSecrets } from './secretStorageAccess'; +import { PerSessionBackupDownloader } from './perSessionBackupDownload'; import type { CryptoEventHandlerMap } from 'matrix-js-sdk/lib/crypto-api/CryptoEventHandlerMap'; import { createDebugLogger } from '$utils/debugLogger'; import { EngineVerificationRequest } from '../verification/request'; @@ -357,10 +358,17 @@ export class EngineCrypto readonly #eventsPendingKey = new Map>(); + readonly #backupDownloader: PerSessionBackupDownloader; + constructor(mx: MatrixClient, identity: EngineIdentity) { super(); this.#mx = mx; this.#identity = identity; + this.#backupDownloader = new PerSessionBackupDownloader({ + mx, + importSession: (roomId, session) => this.#importBackedUpSession(roomId, session), + now: () => Date.now(), + }); // Nothing else drives the backup connection. void this.#connectKeyBackup(); } @@ -500,6 +508,33 @@ export class EngineCrypto const pending = this.#eventsPendingKey.get(key) ?? new Set(); pending.add(event); this.#eventsPendingKey.set(key, pending); + + this.#backupDownloader.request({ roomId, sessionId }); + } + + async #importBackedUpSession(roomId: string, session: KeyBackupSession): Promise { + const backupInfo = await this.getKeyBackupInfo().catch(() => null); + if (!backupInfo?.version) return false; + + const stored = await this.#call('getBackupKeys'); + const privateKey = (stored as { decryptionKeyBase64?: string } | null)?.decryptionKeyBase64; + if (!privateKey) return false; + + const decryptor = await this.getBackupDecryptor(backupInfo, decodeBase64(privateKey)).catch( + () => null + ); + if (!decryptor) return false; + + try { + const decrypted = await decryptor.decryptSessions({ session }); + if (decrypted.length === 0) return false; + + const withRoom = decrypted.map((entry) => ({ ...entry, room_id: roomId })); + const result = await this.#importBackedUpRoomKeys(withRoom, backupInfo.version); + return result.imported > 0; + } finally { + decryptor.free(); + } } async #receiveSyncChanges(input: { @@ -861,6 +896,7 @@ export class EngineCrypto this.#roomsWithTrackedMembers.clear(); this.#encryptionChains.clear(); this.#claimChain = Promise.resolve(); + this.#backupDownloader.stop(); } #trustRequirement(): number { @@ -1844,6 +1880,12 @@ export class EngineCrypto this.emit(CryptoEvent.KeyBackupDecryptionKeyCached, version); } + async requestMissingSecretsIfNeeded(): Promise { + const requested = (await this.#call('requestMissingSecretsIfNeeded')) === true; + if (requested) await this.#flushOutgoingRequests(); + return requested; + } + async checkSecrets(name: string): Promise { const values = ((await this.#call('getSecretsFromInbox', { secretName: name })) ?? []) as string[]; diff --git a/src/app/crypto/engineCrypto/backupKeyFrom4S.test.ts b/src/app/crypto/engineCrypto/backupKeyFrom4S.test.ts index 35853b72c..1eea6d7ff 100644 --- a/src/app/crypto/engineCrypto/backupKeyFrom4S.test.ts +++ b/src/app/crypto/engineCrypto/backupKeyFrom4S.test.ts @@ -5,7 +5,9 @@ import type { MatrixClient } from '$types/matrix-sdk'; import { engineInvoke } from '../olmMachine/engineInvoke'; import { EngineCrypto } from './EngineCrypto'; -vi.mock('../olmMachine/engineInvoke', () => ({ engineInvoke: vi.fn() })); +vi.mock('../olmMachine/engineInvoke', () => ({ + engineInvoke: vi.fn<(...args: never[]) => Promise>(), +})); const mockInvoke = vi.mocked(engineInvoke); @@ -13,7 +15,7 @@ const clientWith = (encoded: string, publicKey: string) => ({ secretStorage: { get: async () => encoded }, http: { - authedRequest: vi.fn(async () => ({ + authedRequest: vi.fn<(...args: never[]) => Promise>(async () => ({ version: '3', algorithm: 'm.megolm_backup.v1.curve25519-aes-sha2', auth_data: { public_key: publicKey }, diff --git a/src/app/crypto/engineCrypto/claimChain.test.ts b/src/app/crypto/engineCrypto/claimChain.test.ts index 4bb4931ae..f73bc4a0b 100644 --- a/src/app/crypto/engineCrypto/claimChain.test.ts +++ b/src/app/crypto/engineCrypto/claimChain.test.ts @@ -3,7 +3,9 @@ import type { MatrixClient, MatrixEvent, Room } from '$types/matrix-sdk'; import { engineInvoke } from '../olmMachine/engineInvoke'; import { EngineCrypto } from './EngineCrypto'; -vi.mock('../olmMachine/engineInvoke', () => ({ engineInvoke: vi.fn() })); +vi.mock('../olmMachine/engineInvoke', () => ({ + engineInvoke: vi.fn<(...args: never[]) => Promise>(), +})); const mockInvoke = vi.mocked(engineInvoke); @@ -20,12 +22,14 @@ const event = () => ({ getType: () => 'm.room.message', getContent: () => ({}), - makeEncrypted: vi.fn(), + makeEncrypted: vi.fn<() => void>(), getTxnId: () => 't', }) as unknown as MatrixEvent; const client = () => - ({ http: { authedRequest: vi.fn(async () => '{}') } }) as unknown as MatrixClient; + ({ + http: { authedRequest: vi.fn<(...args: never[]) => Promise>(async () => '{}') }, + }) as unknown as MatrixClient; describe('key claim serialisation', () => { it('never claims keys for two rooms at the same time', async () => { diff --git a/src/app/crypto/engineCrypto/crossSigningKeys.test.ts b/src/app/crypto/engineCrypto/crossSigningKeys.test.ts index 058ef55ea..68a8a2b38 100644 --- a/src/app/crypto/engineCrypto/crossSigningKeys.test.ts +++ b/src/app/crypto/engineCrypto/crossSigningKeys.test.ts @@ -3,7 +3,9 @@ import type { MatrixClient } from '$types/matrix-sdk'; import { engineInvoke } from '../olmMachine/engineInvoke'; import { EngineCrypto } from './EngineCrypto'; -vi.mock('../olmMachine/engineInvoke', () => ({ engineInvoke: vi.fn() })); +vi.mock('../olmMachine/engineInvoke', () => ({ + engineInvoke: vi.fn<(...args: never[]) => Promise>(), +})); const mockInvoke = vi.mocked(engineInvoke); @@ -11,7 +13,9 @@ const invoked = (method: string) => mockInvoke.mock.calls.filter(([, called]) => const crypto = () => new EngineCrypto( - { http: { authedRequest: vi.fn(async () => '{}') } } as unknown as MatrixClient, + { + http: { authedRequest: vi.fn<(...args: never[]) => Promise>(async () => '{}') }, + } as unknown as MatrixClient, { userId: '@me:e.org', deviceId: 'D', @@ -21,8 +25,6 @@ const crypto = () => describe('userHasCrossSigningKeys', () => { beforeEach(() => mockInvoke.mockReset()); - // Answering false from a stale store makes callers rotate our cross-signing keys and - // invalidate every existing verification, so our own identity is always refreshed. it('refreshes keys/query before answering for our own user', async () => { mockInvoke.mockImplementation(async (_identity, method) => { if (method === 'queryKeysForUsers') return { id: 'q1', type: 1, body: '{}' }; diff --git a/src/app/crypto/engineCrypto/encryptionOrder.test.ts b/src/app/crypto/engineCrypto/encryptionOrder.test.ts index 5b56345d5..1a87d3286 100644 --- a/src/app/crypto/engineCrypto/encryptionOrder.test.ts +++ b/src/app/crypto/engineCrypto/encryptionOrder.test.ts @@ -3,7 +3,9 @@ import type { MatrixClient, MatrixEvent, Room } from '$types/matrix-sdk'; import { engineInvoke } from '../olmMachine/engineInvoke'; import { EngineCrypto } from './EngineCrypto'; -vi.mock('../olmMachine/engineInvoke', () => ({ engineInvoke: vi.fn() })); +vi.mock('../olmMachine/engineInvoke', () => ({ + engineInvoke: vi.fn<(...args: never[]) => Promise>(), +})); const mockInvoke = vi.mocked(engineInvoke); @@ -23,13 +25,11 @@ const event = () => ({ getType: () => 'm.room.message', getContent: () => ({}), - makeEncrypted: vi.fn(), + makeEncrypted: vi.fn<() => void>(), getTxnId: () => 't', }) as unknown as MatrixEvent; describe('encryptEvent ordering', () => { - // element-web#26684: an edit must not overtake the message it edits. Looking members up - // outside the per-room chain reintroduces that race while keeping mutual exclusion. it('does not look up members for the next event until the previous one is encrypted', async () => { let lookups = 0; let release: (() => void) | undefined; @@ -46,7 +46,7 @@ describe('encryptEvent ordering', () => { return null; }); - const mx = { http: { authedRequest: vi.fn() } } as unknown as MatrixClient; + const mx = { http: { authedRequest: vi.fn<() => void>() } } as unknown as MatrixClient; const crypto = new EngineCrypto(mx, { userId: '@me:e.org', deviceId: 'D' }); const target = room('!r:e.org', () => { lookups += 1; @@ -85,7 +85,7 @@ describe('encryptEvent ordering', () => { return null; }); - const mx = { http: { authedRequest: vi.fn() } } as unknown as MatrixClient; + const mx = { http: { authedRequest: vi.fn<() => void>() } } as unknown as MatrixClient; const crypto = new EngineCrypto(mx, { userId: '@me:e.org', deviceId: 'D' }); await Promise.all([ diff --git a/src/app/crypto/engineCrypto/engineShapes.test.ts b/src/app/crypto/engineCrypto/engineShapes.test.ts index 987eb17dd..484a714d6 100644 --- a/src/app/crypto/engineCrypto/engineShapes.test.ts +++ b/src/app/crypto/engineCrypto/engineShapes.test.ts @@ -137,8 +137,6 @@ describe('engine payload shapes', () => { ).resolves.toMatchObject({ privateKeysInSecretStorage: false }); }); - // A secret left behind under a rotated-away 4S key is not recoverable, so reporting it - // as held would tell the user their keys are safe when they are not. it('does not count secrets stored under a key that is no longer the default', async () => { mockInvoke.mockResolvedValue({ hasMaster: false, diff --git a/src/app/crypto/engineCrypto/outgoingRetry.test.ts b/src/app/crypto/engineCrypto/outgoingRetry.test.ts index a7bee5305..615c06efc 100644 --- a/src/app/crypto/engineCrypto/outgoingRetry.test.ts +++ b/src/app/crypto/engineCrypto/outgoingRetry.test.ts @@ -1,12 +1,15 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { ConnectionError, MatrixError, Method } from 'matrix-js-sdk/lib/http-api'; import { sleep } from 'matrix-js-sdk/lib/utils'; +import type * as MatrixUtilsNs from 'matrix-js-sdk/lib/utils'; + +type MatrixUtils = typeof MatrixUtilsNs; import type { MatrixClient } from '$types/matrix-sdk'; import { RequestType, sendOutgoingRequest } from './outgoing'; vi.mock('matrix-js-sdk/lib/utils', async (importOriginal) => ({ - ...(await importOriginal()), - sleep: vi.fn(async () => undefined), + ...(await importOriginal()), + sleep: vi.fn<(ms: number) => Promise>(async () => undefined), })); const rateLimited = (retryAfterMs = 10) => @@ -51,7 +54,7 @@ describe('sendOutgoingRequest', () => { .fn<(...args: never[]) => Promise>() .mockRejectedValue(new MatrixError({ errcode: 'M_UNKNOWN', error: 'boom' }, 500)); - await expect(sendOutgoingRequest(clientWith(authedRequest), request)).rejects.toThrow(); + await expect(sendOutgoingRequest(clientWith(authedRequest), request)).rejects.toThrow('boom'); expect(authedRequest).toHaveBeenCalledTimes(5); }); @@ -60,7 +63,9 @@ describe('sendOutgoingRequest', () => { .fn<(...args: never[]) => Promise>() .mockRejectedValue(new MatrixError({ errcode: 'M_TOO_LARGE', error: 'too big' }, 502)); - await expect(sendOutgoingRequest(clientWith(authedRequest), request)).rejects.toThrow(); + await expect(sendOutgoingRequest(clientWith(authedRequest), request)).rejects.toThrow( + 'too big' + ); expect(authedRequest).toHaveBeenCalledTimes(1); }); @@ -69,7 +74,9 @@ describe('sendOutgoingRequest', () => { .fn<(...args: never[]) => Promise>() .mockRejectedValue(Object.assign(new Error('aborted'), { name: 'AbortError' })); - await expect(sendOutgoingRequest(clientWith(authedRequest), request)).rejects.toThrow(); + await expect(sendOutgoingRequest(clientWith(authedRequest), request)).rejects.toThrow( + 'aborted' + ); expect(authedRequest).toHaveBeenCalledTimes(1); }); diff --git a/src/app/crypto/engineCrypto/perSessionBackupDownload.test.ts b/src/app/crypto/engineCrypto/perSessionBackupDownload.test.ts new file mode 100644 index 000000000..d926905ea --- /dev/null +++ b/src/app/crypto/engineCrypto/perSessionBackupDownload.test.ts @@ -0,0 +1,112 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { KeyBackupSession } from 'matrix-js-sdk/lib/crypto-api/keybackup'; +import type { MatrixClient } from '$types/matrix-sdk'; +import { BACKOFF_TIME_MS, PerSessionBackupDownloader } from './perSessionBackupDownload'; + +const settle = async () => { + for (let i = 0; i < 20; i += 1) { + // eslint-disable-next-line no-await-in-loop + await Promise.resolve(); + } +}; + +const rateLimited = (retryAfterMs: number) => + Object.assign(new Error('slow down'), { + httpStatus: 429, + data: { errcode: 'M_LIMIT_EXCEEDED', retry_after_ms: retryAfterMs }, + }); + +describe('PerSessionBackupDownloader', () => { + let clock = 0; + + beforeEach(() => { + clock = 0; + }); + + const make = ( + authedRequest: ReturnType Promise>>, + importSession = vi.fn<(roomId: string, session: KeyBackupSession) => Promise>( + async () => true + ) + ) => { + const downloader = new PerSessionBackupDownloader({ + mx: { http: { authedRequest } } as unknown as MatrixClient, + importSession, + now: () => clock, + }); + return { downloader, importSession }; + }; + + it('fetches the one missing session and imports it', async () => { + const authedRequest = vi.fn<(...args: never[]) => Promise>(async () => ({ + session_data: {}, + })); + const { downloader, importSession } = make(authedRequest); + + downloader.request({ roomId: '!r:e.org', sessionId: 'S1' }); + await settle(); + + expect(authedRequest).toHaveBeenCalledTimes(1); + expect(authedRequest.mock.calls[0]?.[1]).toBe('/room_keys/keys/!r%3Ae.org/S1'); + expect(importSession).toHaveBeenCalledTimes(1); + }); + + it('does not hammer the backup for a session it is already fetching', async () => { + const authedRequest = vi.fn<(...args: never[]) => Promise>(async () => ({ + session_data: {}, + })); + const { downloader } = make(authedRequest); + + downloader.request({ roomId: '!r:e.org', sessionId: 'S1' }); + downloader.request({ roomId: '!r:e.org', sessionId: 'S1' }); + downloader.request({ roomId: '!r:e.org', sessionId: 'S2' }); + await settle(); + + expect(authedRequest).toHaveBeenCalledTimes(2); + }); + + it('does not re-request a session the backup does not have until the backoff expires', async () => { + const authedRequest = vi.fn<(...args: never[]) => Promise>(async () => { + throw Object.assign(new Error('nope'), { httpStatus: 404, data: {} }); + }); + const { downloader } = make(authedRequest); + + downloader.request({ roomId: '!r:e.org', sessionId: 'S1' }); + await settle(); + downloader.request({ roomId: '!r:e.org', sessionId: 'S1' }); + await settle(); + expect(authedRequest).toHaveBeenCalledTimes(1); + + clock += BACKOFF_TIME_MS + 1; + downloader.request({ roomId: '!r:e.org', sessionId: 'S1' }); + await settle(); + expect(authedRequest).toHaveBeenCalledTimes(2); + }); + + it('stops fetching once told to stop', async () => { + const authedRequest = vi.fn<(...args: never[]) => Promise>(async () => ({ + session_data: {}, + })); + const { downloader } = make(authedRequest); + + downloader.stop(); + downloader.request({ roomId: '!r:e.org', sessionId: 'S1' }); + await settle(); + + expect(authedRequest).not.toHaveBeenCalled(); + }); + + it('re-queues a rate-limited session instead of dropping it', async () => { + const authedRequest = vi + .fn<(...args: never[]) => Promise>() + .mockRejectedValueOnce(rateLimited(0)) + .mockResolvedValue({ session_data: {} }); + const { downloader, importSession } = make(authedRequest); + + downloader.request({ roomId: '!r:e.org', sessionId: 'S1' }); + await settle(); + + expect(authedRequest.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(importSession).toHaveBeenCalled(); + }); +}); diff --git a/src/app/crypto/engineCrypto/perSessionBackupDownload.ts b/src/app/crypto/engineCrypto/perSessionBackupDownload.ts new file mode 100644 index 000000000..12d75eff9 --- /dev/null +++ b/src/app/crypto/engineCrypto/perSessionBackupDownload.ts @@ -0,0 +1,124 @@ +import { ClientPrefix, Method } from 'matrix-js-sdk/lib/http-api'; +import { encodeUri } from 'matrix-js-sdk/lib/utils'; +import type { KeyBackupSession } from 'matrix-js-sdk/lib/crypto-api/keybackup'; +import type { MatrixClient } from '$types/matrix-sdk'; + +export const BACKOFF_TIME_MS = 5000; + +export type SessionRef = { roomId: string; sessionId: string }; + +export type BackupDownloadHost = { + mx: MatrixClient; + importSession: (roomId: string, session: KeyBackupSession) => Promise; + now: () => number; +}; + +export class PerSessionBackupDownloader { + readonly #host: BackupDownloadHost; + + readonly #queue: SessionRef[] = []; + + readonly #queued = new Set(); + + readonly #missingUntil = new Map(); + + #running = false; + + #stopped = false; + + #pausedUntil = 0; + + constructor(host: BackupDownloadHost) { + this.#host = host; + } + + stop(): void { + this.#stopped = true; + this.#queue.length = 0; + this.#queued.clear(); + } + + resume(): void { + this.#missingUntil.clear(); + this.#pausedUntil = 0; + } + + request(ref: SessionRef): void { + if (this.#stopped) return; + + const key = `${ref.roomId}|${ref.sessionId}`; + if (this.#queued.has(key)) return; + + const retryAt = this.#missingUntil.get(key); + if (retryAt !== undefined && this.#host.now() < retryAt) return; + + this.#queued.add(key); + this.#queue.push(ref); + void this.#drain(); + } + + async #drain(): Promise { + if (this.#running) return; + this.#running = true; + + try { + while (!this.#stopped) { + const ref = this.#queue.shift(); + if (!ref) break; + + const wait = this.#pausedUntil - this.#host.now(); + if (wait > 0) { + // eslint-disable-next-line no-await-in-loop + await new Promise((resolve) => { + setTimeout(resolve, wait); + }); + } + if (this.#stopped) break; + + // eslint-disable-next-line no-await-in-loop + await this.#fetchOne(ref); + this.#queued.delete(`${ref.roomId}|${ref.sessionId}`); + } + } finally { + this.#running = false; + } + } + + async #fetchOne(ref: SessionRef): Promise { + const key = `${ref.roomId}|${ref.sessionId}`; + const path = encodeUri('/room_keys/keys/$roomId/$sessionId', { + $roomId: ref.roomId, + $sessionId: ref.sessionId, + }); + + try { + const session = await this.#host.mx.http.authedRequest( + Method.Get, + path, + {}, + undefined, + { prefix: ClientPrefix.V3 } + ); + const imported = await this.#host.importSession(ref.roomId, session); + if (!imported) this.#missingUntil.set(key, this.#host.now() + BACKOFF_TIME_MS); + } catch (error) { + const failure = error as { + httpStatus?: number; + data?: { errcode?: string; retry_after_ms?: number }; + }; + + if (failure.data?.errcode === 'M_LIMIT_EXCEEDED') { + const after = failure.data.retry_after_ms ?? BACKOFF_TIME_MS; + this.#pausedUntil = this.#host.now() + after; + this.#requeue(ref); + return; + } + + this.#missingUntil.set(key, this.#host.now() + BACKOFF_TIME_MS); + } + } + + #requeue(ref: SessionRef): void { + this.#queue.push(ref); + } +} diff --git a/src/app/crypto/install.ts b/src/app/crypto/install.ts index 02d3a39b6..c90f29015 100644 --- a/src/app/crypto/install.ts +++ b/src/app/crypto/install.ts @@ -130,6 +130,10 @@ export const installRustCrypto = async ( cryptoLog.warn('general', 'Failed to read the gossiped backup key', error); }); + engineCrypto.requestMissingSecretsIfNeeded().catch((error: unknown) => { + cryptoLog.warn('general', 'Failed to ask our other devices for missing secrets', error); + }); + const stopEngineCrypto = engineCrypto.stop.bind(engineCrypto); engineCrypto.stop = () => { stopReEmittingCryptoEvents(); diff --git a/src/app/crypto/verification/request.test.ts b/src/app/crypto/verification/request.test.ts index 80282a025..57526deaa 100644 --- a/src/app/crypto/verification/request.test.ts +++ b/src/app/crypto/verification/request.test.ts @@ -30,8 +30,6 @@ const state = (patch: Partial = {}): EngineVerification }); describe('EngineVerificationRequest', () => { - // Both sides press verify at the same moment; the loser's Sas is replaced by a fresh - // one that has not been accepted, and must be re-accepted or the flow hangs. it('re-accepts when our SAS is replaced after losing the start tie-break', async () => { const call = vi.fn<(m: string, a?: Record) => Promise>( async () => null