Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.x] chore: emit info log when buffering, restoring, or clearing future messages #886

Merged
merged 8 commits into from
Jan 27, 2025
2 changes: 1 addition & 1 deletion .github/workflows/benchmarks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/bindings.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/publish-wasm.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions crypto-ffi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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)'] }
2 changes: 1 addition & 1 deletion crypto-ffi/src/generic/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ impl From<CryptoError> 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(),
Expand Down
2 changes: 1 addition & 1 deletion crypto-ffi/src/wasm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ impl From<CryptoError> 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(),
Expand Down
5 changes: 4 additions & 1 deletion crypto/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
10 changes: 8 additions & 2 deletions crypto/src/mls/buffer_external_commit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
wasm_bindgen_test_configure!(run_in_browser);

#[apply(all_cred_cipher)]
#[wasm_bindgen_test]

Check warning on line 44 in crypto/src/mls/buffer_external_commit.rs

View workflow job for this annotation

GitHub Actions / wasm-core-test

unexpected `cfg` condition name: `wasm_bindgen_unstable_test_coverage`

Check warning on line 44 in crypto/src/mls/buffer_external_commit.rs

View workflow job for this annotation

GitHub Actions / wasm-core-test

unexpected `cfg` condition name: `wasm_bindgen_unstable_test_coverage`

Check warning on line 44 in crypto/src/mls/buffer_external_commit.rs

View workflow job for this annotation

GitHub Actions / test

unexpected `cfg` condition name: `wasm_bindgen_unstable_test_coverage`

Check warning on line 44 in crypto/src/mls/buffer_external_commit.rs

View workflow job for this annotation

GitHub Actions / test

unexpected `cfg` condition name: `wasm_bindgen_unstable_test_coverage`
async fn should_buffer_and_reapply_messages_after_external_commit_merged(case: TestCase) {
run_test_with_client_ids(
case.clone(),
Expand Down Expand Up @@ -168,7 +168,7 @@
}

#[apply(all_cred_cipher)]
#[wasm_bindgen_test]

Check warning on line 171 in crypto/src/mls/buffer_external_commit.rs

View workflow job for this annotation

GitHub Actions / wasm-core-test

unexpected `cfg` condition name: `wasm_bindgen_unstable_test_coverage`

Check warning on line 171 in crypto/src/mls/buffer_external_commit.rs

View workflow job for this annotation

GitHub Actions / test

unexpected `cfg` condition name: `wasm_bindgen_unstable_test_coverage`
async fn should_not_reapply_buffered_messages_when_external_commit_contains_remove(case: TestCase) {
run_test_with_client_ids(
case.clone(),
Expand All @@ -192,9 +192,15 @@

// 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;
Expand Down
30 changes: 22 additions & 8 deletions crypto/src/mls/conversation/buffer_messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
//! 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::{
decrypt::MlsBufferedConversationDecryptMessage, Client, ConversationId, CoreCryptoCallbacks, CryptoError,
CryptoResult, MlsConversation, MlsConversationDecryptMessage, MlsError,
CryptoResult, MlsConversation, MlsError,
},
};
use core_crypto_keystore::{
Expand All @@ -25,15 +26,15 @@ impl CentralContext {
&self,
id: &ConversationId,
message: impl AsRef<[u8]>,
) -> CryptoResult<MlsConversationDecryptMessage> {
) -> CryptoResult<()> {
let keystore = self.keystore().await?;

let pending_msg = MlsPendingMessage {
foreign_id: id.clone(),
message: message.as_ref().to_vec(),
};
keystore.save::<MlsPendingMessage>(pending_msg).await?;
Err(CryptoError::BufferedFutureMessage)
Ok(())
}

pub(crate) async fn restore_pending_messages(
Expand Down Expand Up @@ -73,7 +74,6 @@ impl MlsConversation {
is_rejoin: bool,
) -> CryptoResult<Option<Vec<MlsBufferedConversationDecryptMessage>>> {
// 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();
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -216,10 +218,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);
Expand Down Expand Up @@ -343,10 +351,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);
Expand Down
5 changes: 4 additions & 1 deletion crypto/src/mls/conversation/commit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 15 additions & 6 deletions crypto/src/mls/conversation/decrypt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<MlsPendingMessage, _>(self.id()).await?;
Some(pm)
} else {
Expand Down Expand Up @@ -347,7 +348,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,
Expand Down Expand Up @@ -440,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?;
Expand Down Expand Up @@ -1291,7 +1297,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
Expand Down
2 changes: 1 addition & 1 deletion crypto/src/mls/conversation/own_commit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 3 additions & 0 deletions extras/webdriver-installation/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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)'] }
2 changes: 1 addition & 1 deletion keystore/src/connection/platform/generic/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()?)
Expand Down
2 changes: 1 addition & 1 deletion keystore/src/connection/platform/wasm/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading