From ebf1c874837d0de4dadb64e64eb16eb758fcd36b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?bi=C3=B8rn?= Date: Mon, 28 Sep 2026 14:49:58 +0200 Subject: [PATCH] feat: re-add "second device added" message if deleted soon after transfer (#8758) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit this PR makes it easier to spot unwanted "add second device" actions, if the unlocked device is left unattended and/or a system lock is not in place (so cannot be checked directly before doing the "add second device" action). already today, the sending device gets the message "ℹ️ Profile transferred to your second device", which, however, can be deleted. with this PR, if the message is deleted soon after the transfer, it first looks as if this was successful. however, the message is re-added later, at the first housekeeping at least an hour after the transfer. deleting the message after that is final, so users can still get rid of it. It is clear that this does not catch all eventualities, it is best-effort. e.g. it is known that subsequent "add second device" attempts remove checks of previous ones. First line of defense is device locking and asking for secret explicitly before adding a second device. previous discussions about that at https://github.com/chatmail/core/issues/4303 --- src/chat.rs | 13 ++++- src/config.rs | 11 ++++ src/context/context_tests.rs | 2 + src/imex.rs | 1 + src/imex/transfer.rs | 104 ++++++++++++++++++++++++++++++++--- src/sql.rs | 8 ++- src/tools.rs | 1 + 7 files changed, 129 insertions(+), 11 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 1752e54a6..0ec812f00 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -4795,12 +4795,14 @@ pub(crate) async fn get_chat_id_by_grpid( /// /// Optional `label` can be provided to ensure that message is added only once. /// If `important` is true, a notification will be sent. +/// `timestamp_sent` is the time shown on the message; it does not affect ordering. #[expect(clippy::arithmetic_side_effects)] pub async fn add_device_msg_with_importance( context: &Context, label: Option<&str>, msg: Option<&mut Message>, important: bool, + timestamp_sent: i64, ) -> Result { ensure!( label.is_some() || msg.is_some(), @@ -4820,11 +4822,10 @@ pub async fn add_device_msg_with_importance( chat_id = ChatId::get_for_contact(context, ContactId::DEVICE).await?; let rfc724_mid = create_outgoing_rfc724_mid(); - let timestamp_sent = time(); // makes sure, the added message is the last one, // even if the date is wrong (useful esp. when warning about bad dates) - msg.timestamp_sort = timestamp_sent; + msg.timestamp_sort = time(); if let Some(last_msg_time) = chat_id.get_timestamp(context).await? && msg.timestamp_sort <= last_msg_time { @@ -4892,7 +4893,7 @@ pub async fn add_device_msg( label: Option<&str>, msg: Option<&mut Message>, ) -> Result { - add_device_msg_with_importance(context, label, msg, false).await + add_device_msg_with_importance(context, label, msg, false, time()).await } /// Returns true if device message with a given label was ever added to the device chat. @@ -4920,6 +4921,12 @@ pub(crate) async fn delete_and_reset_all_device_msgs(context: &Context) -> Resul .execute("DELETE FROM msgs WHERE from_id=?;", (ContactId::DEVICE,)) .await?; context.sql.execute("DELETE FROM devmsglabels;", ()).await?; + context + .set_config_internal(Config::BackupTransferMsgId, None) + .await?; + context + .set_config_internal(Config::BackupTransferTimestamp, None) + .await?; // Insert labels for welcome messages to avoid them being re-added on reconfiguration. context diff --git a/src/config.rs b/src/config.rs index df8711622..dd08cc49f 100644 --- a/src/config.rs +++ b/src/config.rs @@ -336,6 +336,17 @@ pub enum Config { /// Timestamp of the last time housekeeping was run LastHousekeeping, + /// ID of the device message added after backup transfer. + /// + /// If the message does not exist on the first housekeeping + /// after `READD_BACKUP_TRANSFER_MSG_DELAY`, it is re-added. + BackupTransferMsgId, + + /// Timestamp of the device message added after backup transfer. + /// + /// Needed to check against `READD_BACKUP_TRANSFER_MSG_DELAY`. + BackupTransferTimestamp, + /// Timestamp of the last time accumulated broadcast channel reactions were sent LastReactionsBroadcast, diff --git a/src/context/context_tests.rs b/src/context/context_tests.rs index fb955c584..2906db9c7 100644 --- a/src/context/context_tests.rs +++ b/src/context/context_tests.rs @@ -294,6 +294,8 @@ async fn test_get_info_completeness() { // too sensitive or summarized in another item. let skip_from_get_info = vec![ "addr", + "backup_transfer_msg_id", + "backup_transfer_timestamp", "displayname", "imap_certificate_checks", "mail_server", diff --git a/src/imex.rs b/src/imex.rs index 47a9f9bd3..00afc432c 100644 --- a/src/imex.rs +++ b/src/imex.rs @@ -30,6 +30,7 @@ use crate::tools::{ mod transfer; use ::pgp::types::KeyDetails; +pub(crate) use transfer::maybe_readd_backup_transfer_msg; pub use transfer::{BackupProvider, get_backup}; // Name of the database file in the backup. diff --git a/src/imex/transfer.rs b/src/imex/transfer.rs index be598b86d..0a86a5791 100644 --- a/src/imex/transfer.rs +++ b/src/imex/transfer.rs @@ -40,21 +40,68 @@ use tokio::task::JoinHandle; use tokio_util::sync::CancellationToken; use crate::EventType; -use crate::chat::add_device_msg; +use crate::chat::add_device_msg_with_importance; +use crate::config::Config; use crate::context::Context; use crate::imex::BlobDirContents; use crate::key; use crate::log::warn; -use crate::message::Message; +use crate::message::{Message, MsgId}; use crate::qr::Qr; use crate::stock_str::backup_transfer_msg_body; -use crate::tools::{TempPathGuard, create_id}; +use crate::tools::{TempPathGuard, create_id, time}; use super::{DBFILE_BACKUP_NAME, export_backup_stream, export_database, import_backup_stream}; /// ALPN protocol identifier for the backup transfer protocol. const BACKUP_ALPN: &[u8] = b"/deltachat/backup"; +/// Minimum time after a backup transfer before we check +/// whether the "second device added" device message still exists. +/// +/// The check is done in the first housekeeping after this delay; +/// if the message was deleted until then, it is re-added with the original timestamp. +/// After the check, the user can delete the message as usual without it being re-added. +/// +/// This makes it easier to spot unwanted "add second device" actions. +/// It is clear that this does not catch all eventualities, it is best-effort. +/// First line of defense is device locking and asking for secret explicitly before adding a second device. +const READD_BACKUP_TRANSFER_MSG_DELAY: i64 = 60 * 60; + +/// Checks `Config::BackupTransferMsgId` +/// and re-adds the backup transfer device message if it does not exist and some time passed. +pub(crate) async fn maybe_readd_backup_transfer_msg(context: &Context) -> Result<()> { + let Some(msg_id) = context + .get_config_parsed::(Config::BackupTransferMsgId) + .await? + else { + return Ok(()); + }; + + let timestamp = context + .get_config_i64(Config::BackupTransferTimestamp) + .await?; + if time() < timestamp.saturating_add(READD_BACKUP_TRANSFER_MSG_DELAY) { + return Ok(()); + } + + context + .set_config_internal(Config::BackupTransferMsgId, None) + .await?; + context + .set_config_internal(Config::BackupTransferTimestamp, None) + .await?; + if Message::load_from_db_optional(context, MsgId::new(msg_id)) + .await? + .is_none() + { + let mut msg = Message::new_text(backup_transfer_msg_body(context)); + add_device_msg_with_importance(context, None, Some(&mut msg), false, timestamp).await?; + } + + Ok(()) +} + /// Provide or send a backup of this device. /// /// This creates a backup of the current device and starts a service which offers another @@ -212,8 +259,23 @@ impl BackupProvider { info!(context, "Received backup reception acknowledgement."); context.emit_event(EventType::ImexProgress(1000)); + let timestamp = time(); let mut msg = Message::new_text(backup_transfer_msg_body(&context)); - add_device_msg(&context, None, Some(&mut msg)).await?; + let msg_id = + add_device_msg_with_importance(&context, None, Some(&mut msg), false, timestamp) + .await?; + context + .set_config_internal( + Config::BackupTransferTimestamp, + Some(×tamp.to_string()), + ) + .await?; + context + .set_config_internal( + Config::BackupTransferMsgId, + Some(&msg_id.to_u32().to_string()), + ) + .await?; Ok(()) } @@ -393,14 +455,17 @@ pub async fn get_backup(context: &Context, qr: Qr) -> Result<()> { mod tests { use std::time::Duration; - use crate::chat::{ChatItem, get_chat_msgs, send_msg}; - use crate::message::Viewtype; + use crate::chat::{ChatId, ChatItem, get_chat_msgs, send_msg}; + use crate::contact::ContactId; + use crate::message::{Viewtype, delete_msgs}; + use crate::sql::housekeeping; use crate::test_utils::TestContextManager; + use crate::tools::SystemTime; use super::*; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn test_send_receive() { + async fn test_send_receive() -> Result<()> { let mut tcm = TestContextManager::new(); // Create first device. @@ -472,6 +537,31 @@ mod tests { .get_matching(|ev| matches!(ev, EventType::ImexProgress(1000))) .await; } + + // When deleting the backup transfer message on the sending device, + // it is re-added once on housekeeping, + // but not within the first hour after the transfer. + let device_chat_id = ChatId::get_for_contact(&ctx0, ContactId::DEVICE).await?; + let original_msg = ctx0.get_last_msg_in(device_chat_id).await; + assert_eq!(original_msg.text, backup_transfer_msg_body(&ctx0)); + delete_msgs(&ctx0, &[original_msg.id]).await?; + housekeeping(&ctx0).await?; + assert!(get_chat_msgs(&ctx0, device_chat_id).await?.is_empty()); // re-adding not done in first hour + + SystemTime::shift(Duration::from_secs(60 * 60)); + housekeeping(&ctx0).await?; + let readded_msg = ctx0.get_last_msg_in(device_chat_id).await; + assert_ne!(readded_msg.id, original_msg.id); + assert_eq!(readded_msg.get_text(), original_msg.get_text()); + assert_eq!(readded_msg.get_timestamp(), original_msg.get_timestamp()); + assert!(readded_msg.get_sort_timestamp() > original_msg.get_sort_timestamp()); + + delete_msgs(&ctx0, &[readded_msg.id]).await?; + housekeeping(&ctx0).await?; + let device_msgs = get_chat_msgs(&ctx0, device_chat_id).await?; + assert!(device_msgs.is_empty()); // re-adding is done only once + + Ok(()) } /// Tests that trying to accidentally overwrite a profile diff --git a/src/sql.rs b/src/sql.rs index ff1e722e7..89c7d881e 100644 --- a/src/sql.rs +++ b/src/sql.rs @@ -14,7 +14,7 @@ use crate::config::Config; use crate::context::Context; use crate::debug_logging::set_debug_logging_xdc; use crate::ephemeral::start_ephemeral_timers; -use crate::imex::BLOBS_BACKUP_NAME; +use crate::imex::{self, BLOBS_BACKUP_NAME}; use crate::location; use crate::log::{LogExt, warn}; use crate::message::MsgId; @@ -827,6 +827,12 @@ pub async fn housekeeping(context: &Context) -> Result<()> { ); } + imex::maybe_readd_backup_transfer_msg(context) + .await + .context("Failed to re-add backup transfer message") + .log_err(context) + .ok(); + if let Err(err) = incremental_vacuum(context).await { warn!(context, "Failed to run incremental vacuum: {err:#}."); } diff --git a/src/tools.rs b/src/tools.rs index f4050a49f..381e1b6de 100644 --- a/src/tools.rs +++ b/src/tools.rs @@ -220,6 +220,7 @@ async fn maybe_warn_on_bad_time(context: &Context, now: i64, known_past_timestam ), Some(&mut msg), true, + time(), ) .await .ok();