From 527d001010bceadc51cc0ea4d71eb9904fdbdb94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Wed, 27 Aug 2025 16:31:05 +0200 Subject: [PATCH] fix: Only report duplicate one-time key errors once Since the server will reject any duplicate one-time keys forever, clients which encounter such an error will spam sentry with such reports. This patch ensures that we only send the sentry report once. --- .../src/store/integration_tests.rs | 28 +++++++++++ .../matrix-sdk-base/src/store/memory_store.rs | 11 +++++ crates/matrix-sdk-base/src/store/traits.rs | 15 ++++++ .../src/state_store/mod.rs | 8 ++++ crates/matrix-sdk-sqlite/src/state_store.rs | 9 ++++ crates/matrix-sdk/src/encryption/mod.rs | 48 ++++++++++++++----- 6 files changed, 106 insertions(+), 13 deletions(-) diff --git a/crates/matrix-sdk-base/src/store/integration_tests.rs b/crates/matrix-sdk-base/src/store/integration_tests.rs index e2f17a48a..89ef3512a 100644 --- a/crates/matrix-sdk-base/src/store/integration_tests.rs +++ b/crates/matrix-sdk-base/src/store/integration_tests.rs @@ -73,6 +73,8 @@ pub trait StateStoreIntegrationTests { async fn test_sync_token_saving(&self) -> TestResult; /// Test UtdHookManagerData saving. async fn test_utd_hook_manager_data_saving(&self) -> TestResult; + /// Test the saving of the OneTimeKeyAlreadyUploaded key/value data type. + async fn test_one_time_key_already_uploaded_data_saving(&self) -> TestResult; /// Test stripped room member saving. async fn test_stripped_member_saving(&self) -> TestResult; /// Test room power levels saving. @@ -582,6 +584,26 @@ impl StateStoreIntegrationTests for DynStateStore { .expect("not UtdHookManagerData"); assert_eq!(read_data, data); + + Ok(()) + } + + async fn test_one_time_key_already_uploaded_data_saving(&self) -> TestResult { + // Before any data is written, the getter should return None. + assert!( + self.get_kv_data(StateStoreDataKey::OneTimeKeyAlreadyUploaded).await?.is_none(), + "Store was not empty at start" + ); + + self.set_kv_data( + StateStoreDataKey::OneTimeKeyAlreadyUploaded, + StateStoreDataValue::OneTimeKeyAlreadyUploaded, + ) + .await?; + + let data = self.get_kv_data(StateStoreDataKey::OneTimeKeyAlreadyUploaded).await?; + data.expect("The loaded data should be Some"); + Ok(()) } @@ -1904,6 +1926,12 @@ macro_rules! statestore_integration_tests { store.test_utd_hook_manager_data_saving().await } + #[async_test] + async fn test_one_time_key_already_uploaded_data_saving() -> TestResult { + let store = get_store().await?.into_state_store(); + store.test_one_time_key_already_uploaded_data_saving().await + } + #[async_test] async fn test_stripped_member_saving() -> TestResult { let store = get_store().await?.into_state_store(); diff --git a/crates/matrix-sdk-base/src/store/memory_store.rs b/crates/matrix-sdk-base/src/store/memory_store.rs index fdf3676b9..14deda1e1 100644 --- a/crates/matrix-sdk-base/src/store/memory_store.rs +++ b/crates/matrix-sdk-base/src/store/memory_store.rs @@ -58,6 +58,7 @@ struct MemoryStoreInner { server_info: Option, filters: HashMap, utd_hook_manager_data: Option, + one_time_key_uploaded_error: bool, account_data: HashMap>, profiles: HashMap>, display_names: HashMap>>, @@ -146,6 +147,7 @@ impl StateStore for MemoryStore { async fn get_kv_data(&self, key: StateStoreDataKey<'_>) -> Result> { let inner = self.inner.read().unwrap(); + Ok(match key { StateStoreDataKey::SyncToken => { inner.sync_token.clone().map(StateStoreDataValue::SyncToken) @@ -167,6 +169,9 @@ impl StateStore for MemoryStore { StateStoreDataKey::UtdHookManagerData => { inner.utd_hook_manager_data.clone().map(StateStoreDataValue::UtdHookManagerData) } + StateStoreDataKey::OneTimeKeyAlreadyUploaded => inner + .one_time_key_uploaded_error + .then_some(StateStoreDataValue::OneTimeKeyAlreadyUploaded), StateStoreDataKey::ComposerDraft(room_id, thread_root) => { let key = (room_id.to_owned(), thread_root.map(ToOwned::to_owned)); inner.composer_drafts.get(&key).cloned().map(StateStoreDataValue::ComposerDraft) @@ -217,6 +222,9 @@ impl StateStore for MemoryStore { .expect("Session data not the hook manager data"), ); } + StateStoreDataKey::OneTimeKeyAlreadyUploaded => { + inner.one_time_key_uploaded_error = true; + } StateStoreDataKey::ComposerDraft(room_id, thread_root) => { inner.composer_drafts.insert( (room_id.to_owned(), thread_root.map(ToOwned::to_owned)), @@ -256,6 +264,9 @@ impl StateStore for MemoryStore { inner.recently_visited_rooms.remove(user_id); } StateStoreDataKey::UtdHookManagerData => inner.utd_hook_manager_data = None, + StateStoreDataKey::OneTimeKeyAlreadyUploaded => { + inner.one_time_key_uploaded_error = false + } StateStoreDataKey::ComposerDraft(room_id, thread_root) => { let key = (room_id.to_owned(), thread_root.map(ToOwned::to_owned)); inner.composer_drafts.remove(&key); diff --git a/crates/matrix-sdk-base/src/store/traits.rs b/crates/matrix-sdk-base/src/store/traits.rs index 3904c477d..f74df591b 100644 --- a/crates/matrix-sdk-base/src/store/traits.rs +++ b/crates/matrix-sdk-base/src/store/traits.rs @@ -1131,6 +1131,10 @@ pub enum StateStoreDataValue { /// `matrix_sdk_ui::unable_to_decrypt_hook::UtdHookManager`. UtdHookManagerData(GrowableBloom), + /// A unit value telling us that the client uploaded duplicate one-time + /// keys. + OneTimeKeyAlreadyUploaded, + /// A composer draft for the room. /// To learn more, see [`ComposerDraft`]. /// @@ -1234,6 +1238,10 @@ pub enum StateStoreDataKey<'a> { /// `matrix_sdk_ui::unable_to_decrypt_hook::UtdHookManager`. UtdHookManagerData, + /// Data remembering if the client already reported that it has uploaded + /// duplicate one-time keys. + OneTimeKeyAlreadyUploaded, + /// A composer draft for the room. /// To learn more, see [`ComposerDraft`]. /// @@ -1247,11 +1255,14 @@ pub enum StateStoreDataKey<'a> { impl StateStoreDataKey<'_> { /// Key to use for the [`SyncToken`][Self::SyncToken] variant. pub const SYNC_TOKEN: &'static str = "sync_token"; + /// Key to use for the [`ServerInfo`][Self::ServerInfo] /// variant. pub const SERVER_INFO: &'static str = "server_capabilities"; // Note: this is the old name, kept for backwards compatibility. + // /// Key prefix to use for the [`Filter`][Self::Filter] variant. pub const FILTER: &'static str = "filter"; + /// Key prefix to use for the [`UserAvatarUrl`][Self::UserAvatarUrl] /// variant. pub const USER_AVATAR_URL: &'static str = "user_avatar_url"; @@ -1264,6 +1275,10 @@ impl StateStoreDataKey<'_> { /// variant. pub const UTD_HOOK_MANAGER_DATA: &'static str = "utd_hook_manager_data"; + /// Key to use for the flag remembering that we already reported that we + /// uploaded duplicate one-time keys. + pub const ONE_TIME_KEY_ALREADY_UPLOADED: &'static str = "one_time_key_already_uploaded"; + /// Key prefix to use for the [`ComposerDraft`][Self::ComposerDraft] /// variant. pub const COMPOSER_DRAFT: &'static str = "composer_draft"; diff --git a/crates/matrix-sdk-indexeddb/src/state_store/mod.rs b/crates/matrix-sdk-indexeddb/src/state_store/mod.rs index 227f0496a..c5b8e9b0a 100644 --- a/crates/matrix-sdk-indexeddb/src/state_store/mod.rs +++ b/crates/matrix-sdk-indexeddb/src/state_store/mod.rs @@ -422,6 +422,9 @@ impl IndexeddbStateStore { StateStoreDataKey::UtdHookManagerData => { self.encode_key(keys::KV, StateStoreDataKey::UTD_HOOK_MANAGER_DATA) } + StateStoreDataKey::OneTimeKeyAlreadyUploaded => { + self.encode_key(keys::KV, StateStoreDataKey::ONE_TIME_KEY_ALREADY_UPLOADED) + } StateStoreDataKey::ComposerDraft(room_id, thread_root) => { if let Some(thread_root) = thread_root { self.encode_key( @@ -563,6 +566,10 @@ impl_state_store!({ .map(|f| self.deserialize_value::(&f)) .transpose()? .map(StateStoreDataValue::UtdHookManagerData), + StateStoreDataKey::OneTimeKeyAlreadyUploaded => value + .map(|f| self.deserialize_value::(&f)) + .transpose()? + .map(|_| StateStoreDataValue::OneTimeKeyAlreadyUploaded), StateStoreDataKey::ComposerDraft(_, _) => value .map(|f| self.deserialize_value::(&f)) .transpose()? @@ -603,6 +610,7 @@ impl_state_store!({ StateStoreDataKey::UtdHookManagerData => self.serialize_value( &value.into_utd_hook_manager_data().expect("Session data not UtdHookManagerData"), ), + StateStoreDataKey::OneTimeKeyAlreadyUploaded => self.serialize_value(&true), StateStoreDataKey::ComposerDraft(_, _) => self.serialize_value( &value.into_composer_draft().expect("Session data not a composer draft"), ), diff --git a/crates/matrix-sdk-sqlite/src/state_store.rs b/crates/matrix-sdk-sqlite/src/state_store.rs index 4729f1fa1..74188f0b9 100644 --- a/crates/matrix-sdk-sqlite/src/state_store.rs +++ b/crates/matrix-sdk-sqlite/src/state_store.rs @@ -387,6 +387,9 @@ impl SqliteStateStore { StateStoreDataKey::UtdHookManagerData => { Cow::Borrowed(StateStoreDataKey::UTD_HOOK_MANAGER_DATA) } + StateStoreDataKey::OneTimeKeyAlreadyUploaded => { + Cow::Borrowed(StateStoreDataKey::ONE_TIME_KEY_ALREADY_UPLOADED) + } StateStoreDataKey::ComposerDraft(room_id, thread_root) => { if let Some(thread_root) = thread_root { Cow::Owned(format!( @@ -1012,6 +1015,9 @@ impl StateStore for SqliteStateStore { StateStoreDataKey::UtdHookManagerData => { StateStoreDataValue::UtdHookManagerData(self.deserialize_value(&data)?) } + StateStoreDataKey::OneTimeKeyAlreadyUploaded => { + StateStoreDataValue::OneTimeKeyAlreadyUploaded + } StateStoreDataKey::ComposerDraft(_, _) => { StateStoreDataValue::ComposerDraft(self.deserialize_value(&data)?) } @@ -1047,6 +1053,9 @@ impl StateStore for SqliteStateStore { StateStoreDataKey::UtdHookManagerData => self.serialize_value( &value.into_utd_hook_manager_data().expect("Session data not UtdHookManagerData"), )?, + StateStoreDataKey::OneTimeKeyAlreadyUploaded => { + self.serialize_value(&true).expect("We should be able to serialize a boolean") + } StateStoreDataKey::ComposerDraft(_, _) => self.serialize_value( &value.into_composer_draft().expect("Session data not a composer draft"), )?, diff --git a/crates/matrix-sdk/src/encryption/mod.rs b/crates/matrix-sdk/src/encryption/mod.rs index 126e19e45..e7dd5e5d9 100644 --- a/crates/matrix-sdk/src/encryption/mod.rs +++ b/crates/matrix-sdk/src/encryption/mod.rs @@ -34,12 +34,15 @@ use futures_util::{ }; #[cfg(feature = "experimental-send-custom-to-device")] use matrix_sdk_base::crypto::CollectStrategy; -use matrix_sdk_base::crypto::{ - store::types::{RoomKeyBundleInfo, RoomKeyInfo}, - types::requests::{ - OutgoingRequest, OutgoingVerificationRequest, RoomMessageRequest, ToDeviceRequest, +use matrix_sdk_base::{ + crypto::{ + store::types::{RoomKeyBundleInfo, RoomKeyInfo}, + types::requests::{ + OutgoingRequest, OutgoingVerificationRequest, RoomMessageRequest, ToDeviceRequest, + }, + CrossSigningBootstrapRequests, OlmMachine, }, - CrossSigningBootstrapRequests, OlmMachine, + StateStoreDataKey, StateStoreDataValue, }; use matrix_sdk_common::{executor::spawn, locks::Mutex as StdMutex}; use ruma::{ @@ -623,7 +626,9 @@ impl Client { self.keys_query(r.request_id(), request.device_keys.clone()).await?; } AnyOutgoingRequest::KeysUpload(request) => { - self.keys_upload(r.request_id(), request).await.inspect_err(|e| { + let response = self.keys_upload(r.request_id(), request).await; + + if let Err(e) = &response { match e.as_ruma_api_error() { Some(RumaApiError::ClientApi(e)) if e.status_code == 400 => { if let ErrorBody::Standard { message, .. } = &e.body { @@ -631,18 +636,35 @@ impl Client { // telling us that we already have a one-time key uploaded means // that we forgot about some of our one-time keys. This will lead to // UTDs. - if message.starts_with("One time key") { - tracing::error!( - sentry = true, - error_message = message, - "Duplicate one-time keys have been uploaded" - ); + { + let already_reported = self + .state_store() + .get_kv_data(StateStoreDataKey::OneTimeKeyAlreadyUploaded) + .await? + .is_some(); + + if message.starts_with("One time key") && !already_reported { + tracing::error!( + sentry = true, + error_message = message, + "Duplicate one-time keys have been uploaded" + ); + + self.state_store() + .set_kv_data( + StateStoreDataKey::OneTimeKeyAlreadyUploaded, + StateStoreDataValue::OneTimeKeyAlreadyUploaded, + ) + .await?; + } } } } _ => {} } - })?; + + response?; + } } AnyOutgoingRequest::ToDeviceRequest(request) => { let response = self.send_to_device(request).await?;