From d82b3dde3d520dda0b05fc249124e3cdd6e98cd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Stankovi=C4=87?= Date: Mon, 13 Jan 2025 13:50:02 +0100 Subject: [PATCH 1/8] chore: crypto-ffi: silence warning about unexpected cfg in wasm-bindgen (cherry picked from commit e6d3e9024e317fa46df39808671e2a743323c19e) --- crypto-ffi/Cargo.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crypto-ffi/Cargo.toml b/crypto-ffi/Cargo.toml index d01a5bd9b8..2b4cf63c2d 100644 --- a/crypto-ffi/Cargo.toml +++ b/crypto-ffi/Cargo.toml @@ -69,3 +69,6 @@ wasm-opt = false [package.metadata.wasm-pack.profile.release] wasm-opt = ["-Os", "--enable-mutable-globals", "--enable-threads", "--detect-features"] + +[lints.rust] +unexpected_cfgs = { level = "warn", check-cfg = ['cfg(wasm_bindgen_unstable_test_coverage)'] } From f23917d2e2edbc979c123b3ece8a415698bf8c65 Mon Sep 17 00:00:00 2001 From: SimonThormeyer Date: Mon, 27 Jan 2025 11:57:19 +0100 Subject: [PATCH 2/8] chore: silence more warnings --- extras/webdriver-installation/Cargo.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/extras/webdriver-installation/Cargo.toml b/extras/webdriver-installation/Cargo.toml index a268aeca87..3c898c2e90 100644 --- a/extras/webdriver-installation/Cargo.toml +++ b/extras/webdriver-installation/Cargo.toml @@ -38,3 +38,6 @@ zip = "0.6" flate2 = "1.0" tar = "0.4" tracing = "0.1" + +[lints.rust] +unexpected_cfgs = { level = "warn", check-cfg = ['cfg(trick_rust_analyzer_into_highlighting_interpolated_bits)'] } From f55b6591917462ae98ffabaf401b0369216a2b00 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 13 Jan 2025 15:07:30 +0100 Subject: [PATCH 3/8] chore: fix clippy warnings about needless lifetimes so CI can build cleanly again With most new rustc versions come new lints. These are great, because they cause our codebase to gradually improve. But they necessitate commits like this from time to time where a lint got stricter. (cherry picked from commit a7ac605f5a804eb59804f8fc8656ba361854c165) --- crypto/src/mls/conversation/own_commit.rs | 2 +- keystore/src/connection/platform/generic/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crypto/src/mls/conversation/own_commit.rs b/crypto/src/mls/conversation/own_commit.rs index 100231ddff..633163da5c 100644 --- a/crypto/src/mls/conversation/own_commit.rs +++ b/crypto/src/mls/conversation/own_commit.rs @@ -49,7 +49,7 @@ impl MlsConversation { } } - pub(crate) async fn handle_own_commit<'a>( + pub(crate) async fn handle_own_commit( &mut self, backend: &MlsCryptoProvider, ct: &ConfirmationTag, diff --git a/keystore/src/connection/platform/generic/mod.rs b/keystore/src/connection/platform/generic/mod.rs index 37ff95c043..0afdac64f3 100644 --- a/keystore/src/connection/platform/generic/mod.rs +++ b/keystore/src/connection/platform/generic/mod.rs @@ -32,7 +32,7 @@ pub struct SqlCipherConnection { pub struct TransactionWrapper<'conn> { transaction: Transaction<'conn>, } -impl<'conn> TransactionWrapper<'conn> { +impl TransactionWrapper<'_> { // this is async just to conform with the wasm impl pub(crate) async fn commit_tx(self) -> CryptoKeystoreResult<()> { Ok(self.transaction.commit()?) From df18e25b6f0cebabfc4518be4cab57d39b87e145 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 13 Jan 2025 15:43:09 +0100 Subject: [PATCH 4/8] chore: fix new clippy warning in wasm also (cherry picked from commit 7049e08216115f83d0a0a7e7ed4595158a8f19cd) --- keystore/src/connection/platform/wasm/storage.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/keystore/src/connection/platform/wasm/storage.rs b/keystore/src/connection/platform/wasm/storage.rs index 79bb935d97..04cbbfe5a4 100644 --- a/keystore/src/connection/platform/wasm/storage.rs +++ b/keystore/src/connection/platform/wasm/storage.rs @@ -45,7 +45,7 @@ pub enum WasmStorageTransaction<'a> { }, } -impl<'a> WasmStorageTransaction<'a> { +impl WasmStorageTransaction<'_> { pub(crate) async fn commit_tx(self) -> CryptoKeystoreResult<()> { let Self::Persistent { tx: transaction, From 3a62106e876b683db3620b9c8daca795742f2813 Mon Sep 17 00:00:00 2001 From: SimonThormeyer Date: Mon, 27 Jan 2025 10:47:06 +0100 Subject: [PATCH 5/8] chore: add the epoch of the buffered message to the buffered future message error This is to enable logging the epoch after buffering a message. --- crypto-ffi/src/generic/mod.rs | 2 +- crypto-ffi/src/wasm/mod.rs | 2 +- crypto/src/error.rs | 5 ++++- crypto/src/mls/buffer_external_commit.rs | 10 ++++++++-- .../src/mls/conversation/buffer_messages.rs | 20 +++++++++++++++---- crypto/src/mls/conversation/commit.rs | 5 ++++- crypto/src/mls/conversation/decrypt.rs | 9 +++++++-- 7 files changed, 41 insertions(+), 12 deletions(-) diff --git a/crypto-ffi/src/generic/mod.rs b/crypto-ffi/src/generic/mod.rs index 7559b28d34..9839112786 100644 --- a/crypto-ffi/src/generic/mod.rs +++ b/crypto-ffi/src/generic/mod.rs @@ -209,7 +209,7 @@ impl From for CoreCryptoError { match value { CryptoError::ConversationAlreadyExists(id) => MlsError::ConversationAlreadyExists(id).into(), CryptoError::DuplicateMessage => MlsError::DuplicateMessage.into(), - CryptoError::BufferedFutureMessage => MlsError::BufferedFutureMessage.into(), + CryptoError::BufferedFutureMessage { .. } => MlsError::BufferedFutureMessage.into(), CryptoError::WrongEpoch => MlsError::WrongEpoch.into(), CryptoError::MessageEpochTooOld => MlsError::MessageEpochTooOld.into(), CryptoError::SelfCommitIgnored => MlsError::SelfCommitIgnored.into(), diff --git a/crypto-ffi/src/wasm/mod.rs b/crypto-ffi/src/wasm/mod.rs index 7f4f88cf54..7ad84f6796 100644 --- a/crypto-ffi/src/wasm/mod.rs +++ b/crypto-ffi/src/wasm/mod.rs @@ -211,7 +211,7 @@ impl From for InternalError { match value { CryptoError::ConversationAlreadyExists(id) => MlsError::ConversationAlreadyExists(id).into(), CryptoError::DuplicateMessage => MlsError::DuplicateMessage.into(), - CryptoError::BufferedFutureMessage => MlsError::BufferedFutureMessage.into(), + CryptoError::BufferedFutureMessage { .. } => MlsError::BufferedFutureMessage.into(), CryptoError::WrongEpoch => MlsError::WrongEpoch.into(), CryptoError::MessageEpochTooOld => MlsError::MessageEpochTooOld.into(), CryptoError::SelfCommitIgnored => MlsError::SelfCommitIgnored.into(), diff --git a/crypto/src/error.rs b/crypto/src/error.rs index ed784cd675..61083f7db4 100644 --- a/crypto/src/error.rs +++ b/crypto/src/error.rs @@ -139,7 +139,10 @@ pub enum CryptoError { WrongEpoch, /// Incoming message is for a future epoch. We will buffer it until the commit for that epoch arrives #[error("Incoming message is for a future epoch. We will buffer it until the commit for that epoch arrives")] - BufferedFutureMessage, + BufferedFutureMessage { + /// The epoch of the buffered message + message_epoch: u64, + }, /// Proteus Error Wrapper #[error(transparent)] ProteusError(#[from] ProteusError), diff --git a/crypto/src/mls/buffer_external_commit.rs b/crypto/src/mls/buffer_external_commit.rs index 11fd30edc9..5d7502446e 100644 --- a/crypto/src/mls/buffer_external_commit.rs +++ b/crypto/src/mls/buffer_external_commit.rs @@ -192,9 +192,15 @@ mod tests { // Since Alice missed Bob's commit she should buffer this message let decrypt = alice_central.context.decrypt_message(&id, msg1).await; - assert!(matches!(decrypt.unwrap_err(), CryptoError::BufferedFutureMessage)); + assert!(matches!( + decrypt.unwrap_err(), + CryptoError::BufferedFutureMessage { .. } + )); let decrypt = alice_central.context.decrypt_message(&id, msg2).await; - assert!(matches!(decrypt.unwrap_err(), CryptoError::BufferedFutureMessage)); + assert!(matches!( + decrypt.unwrap_err(), + CryptoError::BufferedFutureMessage { .. } + )); assert_eq!(alice_central.context.count_entities().await.pending_messages, 2); let gi = bob_central.get_group_info(&id).await; diff --git a/crypto/src/mls/conversation/buffer_messages.rs b/crypto/src/mls/conversation/buffer_messages.rs index b28167ece6..afa9d96578 100644 --- a/crypto/src/mls/conversation/buffer_messages.rs +++ b/crypto/src/mls/conversation/buffer_messages.rs @@ -216,10 +216,16 @@ mod tests { .map(|m| m.to_bytes().unwrap()); for m in messages { let decrypt = bob_central.context.decrypt_message(&id, m).await; - assert!(matches!(decrypt.unwrap_err(), CryptoError::BufferedFutureMessage)); + assert!(matches!( + decrypt.unwrap_err(), + CryptoError::BufferedFutureMessage { .. } + )); } let decrypt = bob_central.context.decrypt_message(&id, app_msg).await; - assert!(matches!(decrypt.unwrap_err(), CryptoError::BufferedFutureMessage)); + assert!(matches!( + decrypt.unwrap_err(), + CryptoError::BufferedFutureMessage { .. } + )); // Bob should have buffered the messages assert_eq!(bob_central.context.count_entities().await.pending_messages, 4); @@ -343,10 +349,16 @@ mod tests { .map(|m| m.to_bytes().unwrap()); for m in messages { let decrypt = alice_central.context.decrypt_message(&id, m).await; - assert!(matches!(decrypt.unwrap_err(), CryptoError::BufferedFutureMessage)); + assert!(matches!( + decrypt.unwrap_err(), + CryptoError::BufferedFutureMessage { .. } + )); } let decrypt = alice_central.context.decrypt_message(&id, app_msg).await; - assert!(matches!(decrypt.unwrap_err(), CryptoError::BufferedFutureMessage)); + assert!(matches!( + decrypt.unwrap_err(), + CryptoError::BufferedFutureMessage { .. } + )); // Alice should have buffered the messages assert_eq!(alice_central.context.count_entities().await.pending_messages, 4); diff --git a/crypto/src/mls/conversation/commit.rs b/crypto/src/mls/conversation/commit.rs index 13dfd49ed6..a0b7b23b87 100644 --- a/crypto/src/mls/conversation/commit.rs +++ b/crypto/src/mls/conversation/commit.rs @@ -1112,7 +1112,10 @@ mod tests { // fails when a commit is skipped let out_of_order = bob_central.context.decrypt_message(&id, &commit2).await; - assert!(matches!(out_of_order.unwrap_err(), CryptoError::BufferedFutureMessage)); + assert!(matches!( + out_of_order.unwrap_err(), + CryptoError::BufferedFutureMessage { .. } + )); // works in the right order though // NB: here 'commit2' has been buffered so it is also applied when we decrypt commit1 diff --git a/crypto/src/mls/conversation/decrypt.rs b/crypto/src/mls/conversation/decrypt.rs index f4a1899f0b..848fc68c10 100644 --- a/crypto/src/mls/conversation/decrypt.rs +++ b/crypto/src/mls/conversation/decrypt.rs @@ -347,7 +347,9 @@ impl MlsConversation { } else if msg_epoch == group_epoch + 1 { // limit to next epoch otherwise if we were buffering a commit for epoch + 2 // we would fail when trying to decrypt it in [MlsCentral::commit_accepted] - CryptoError::BufferedFutureMessage + CryptoError::BufferedFutureMessage { + message_epoch: msg_epoch, + } } else if msg_epoch < group_epoch { match content_type { ContentType::Application => CryptoError::StaleMessage, @@ -1291,7 +1293,10 @@ mod tests { // which Bob cannot decrypt because of Post CompromiseSecurity let decrypt = bob_central.context.decrypt_message(&id, &encrypted).await; - assert!(matches!(decrypt.unwrap_err(), CryptoError::BufferedFutureMessage)); + assert!(matches!( + decrypt.unwrap_err(), + CryptoError::BufferedFutureMessage { .. } + )); let decrypted_commit = bob_central .context From 8e3376b35a460b0840da961cc011b3fe0fc79a1a Mon Sep 17 00:00:00 2001 From: SimonThormeyer Date: Mon, 27 Jan 2025 10:51:15 +0100 Subject: [PATCH 6/8] refactor: don't return `Err(Error::BufferedFutureMessage)` from `handle_future_message()` This was meaningless because in the one and only caller this error would always be propagated. So we can just keep it in that context and raise it from there directly. No need for an extra round trip. (cherry picked and adjusted from commit 5d7af36053ca4921ee40b759f0423206e73215c1) --- crypto/src/mls/conversation/buffer_messages.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crypto/src/mls/conversation/buffer_messages.rs b/crypto/src/mls/conversation/buffer_messages.rs index afa9d96578..c3fda74a81 100644 --- a/crypto/src/mls/conversation/buffer_messages.rs +++ b/crypto/src/mls/conversation/buffer_messages.rs @@ -8,7 +8,7 @@ use crate::{ group_store::GroupStoreValue, prelude::{ decrypt::MlsBufferedConversationDecryptMessage, Client, ConversationId, CoreCryptoCallbacks, CryptoError, - CryptoResult, MlsConversation, MlsConversationDecryptMessage, MlsError, + CryptoResult, MlsConversation, MlsError, }, }; use core_crypto_keystore::{ @@ -25,7 +25,7 @@ impl CentralContext { &self, id: &ConversationId, message: impl AsRef<[u8]>, - ) -> CryptoResult { + ) -> CryptoResult<()> { let keystore = self.keystore().await?; let pending_msg = MlsPendingMessage { @@ -33,7 +33,7 @@ impl CentralContext { message: message.as_ref().to_vec(), }; keystore.save::(pending_msg).await?; - Err(CryptoError::BufferedFutureMessage) + Ok(()) } pub(crate) async fn restore_pending_messages( From a74bac93589da62f83baa85f5aebbe4f84473b6b Mon Sep 17 00:00:00 2001 From: SimonThormeyer Date: Mon, 27 Jan 2025 10:48:39 +0100 Subject: [PATCH 7/8] chore: emit info log when buffering, restoring, or clearing future messages (cherry picked and adapted from commit 08b45637ebab205595910b1ef23eb924d47a4653) --- crypto/src/mls/conversation/buffer_messages.rs | 4 +++- crypto/src/mls/conversation/decrypt.rs | 12 ++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/crypto/src/mls/conversation/buffer_messages.rs b/crypto/src/mls/conversation/buffer_messages.rs index c3fda74a81..de06eea887 100644 --- a/crypto/src/mls/conversation/buffer_messages.rs +++ b/crypto/src/mls/conversation/buffer_messages.rs @@ -4,6 +4,7 @@ //! Feel free to delete all of this when the issue is fixed on the DS side ! use crate::context::CentralContext; +use crate::obfuscate::Obfuscated; use crate::{ group_store::GroupStoreValue, prelude::{ @@ -73,7 +74,6 @@ impl MlsConversation { is_rejoin: bool, ) -> CryptoResult>> { // using the macro produces a clippy warning - info!("restore_pending_messages"); let result = async move { let keystore = backend.keystore(); let group_id = self.id().as_slice(); @@ -109,6 +109,8 @@ impl MlsConversation { // luckily for us that's the exact same order as the [ContentType] enum pending_messages.sort_by(|(a, _), (b, _)| a.cmp(b)); + info!(group_id = Obfuscated::from(&self.id); "Attempting to restore {} buffered messages", pending_messages.len()); + let mut decrypted_messages = Vec::with_capacity(pending_messages.len()); for (_, m) in pending_messages { let parent_conversation = match &self.parent_id { diff --git a/crypto/src/mls/conversation/decrypt.rs b/crypto/src/mls/conversation/decrypt.rs index 848fc68c10..c11a0c6c20 100644 --- a/crypto/src/mls/conversation/decrypt.rs +++ b/crypto/src/mls/conversation/decrypt.rs @@ -248,6 +248,7 @@ impl MlsConversation { .restore_pending_messages(client, backend, callbacks, parent_conv, false) .await? { + info!(group_id = Obfuscated::from(&self.id); "Clearing all buffered messages for conversation"); backend.key_store().remove::(self.id()).await?; Some(pm) } else { @@ -442,10 +443,13 @@ impl CentralContext { ) .await; - let decrypt_message = match decrypt_message { - Err(CryptoError::BufferedFutureMessage) => self.handle_future_message(id, message).await?, - _ => decrypt_message?, - }; + if let Err(CryptoError::BufferedFutureMessage { message_epoch }) = decrypt_message { + self.handle_future_message(id, message).await?; + info!(group_id = Obfuscated::from(id); "Buffered future message from epoch {message_epoch}"); + return decrypt_message; + } + + let decrypt_message = decrypt_message?; if !decrypt_message.is_active { self.wipe_conversation(id).await?; From 740e7e466b51f451c3bb8df08b414d781d3da3fb Mon Sep 17 00:00:00 2001 From: SimonThormeyer Date: Mon, 27 Jan 2025 13:04:46 +0100 Subject: [PATCH 8/8] chore: use bun 1.1 The recent 1.2 release apparently had some breaking changes for us. --- .github/workflows/benchmarks.yml | 2 +- .github/workflows/bindings.yml | 2 +- .github/workflows/publish-wasm.yml | 2 +- .github/workflows/rust.yml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/benchmarks.yml b/.github/workflows/benchmarks.yml index 3fac0537a8..23ed3082e4 100644 --- a/.github/workflows/benchmarks.yml +++ b/.github/workflows/benchmarks.yml @@ -37,7 +37,7 @@ jobs: - name: Setup bun (web) uses: oven-sh/setup-bun@v2 with: - bun-version: latest + bun-version: 1.1 - name: Install wasm-pack (web) uses: taiki-e/install-action@v2 with: diff --git a/.github/workflows/bindings.yml b/.github/workflows/bindings.yml index 5f93dc4a4c..ae13f17b5d 100644 --- a/.github/workflows/bindings.yml +++ b/.github/workflows/bindings.yml @@ -111,7 +111,7 @@ jobs: target: wasm32-unknown-unknown - uses: oven-sh/setup-bun@v2 with: - bun-version: latest + bun-version: 1.1 - uses: actions/setup-node@v4 with: node-version: 18 diff --git a/.github/workflows/publish-wasm.yml b/.github/workflows/publish-wasm.yml index ac0d979579..36391f68b3 100644 --- a/.github/workflows/publish-wasm.yml +++ b/.github/workflows/publish-wasm.yml @@ -29,7 +29,7 @@ jobs: - uses: oven-sh/setup-bun@v2 with: - bun-version: latest + bun-version: 1.1 - uses: actions-rust-lang/setup-rust-toolchain@v1 # this implicitly caches Rust tools and build artifacts with: diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 41de04b213..80a33469ad 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -201,7 +201,7 @@ jobs: chrome-version: stable - uses: oven-sh/setup-bun@v2 with: - bun-version: latest + bun-version: 1.1 - uses: actions/setup-node@v4 with: node-version: 18