From 84c1a6b8feb15b7fe91b9522f6a845e37a2b4304 Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 31 Jul 2026 13:30:13 -0500 Subject: [PATCH 1/4] fix(cli): render clean error messages instead of raw tonic::Status (#336) Replace ~28 CLI error paths that formatted a tonic::Status directly (dumping its Debug output, including internal metadata) with the server-provided status.message(), falling back to a short " error" form when the message is empty. Refs #321 Co-authored-by: James Maes --- crates/nuncio-cli/src/runner.rs | 293 ++++++++++++++++++++++++++++---- 1 file changed, 259 insertions(+), 34 deletions(-) diff --git a/crates/nuncio-cli/src/runner.rs b/crates/nuncio-cli/src/runner.rs index f14f0de..912f6b7 100644 --- a/crates/nuncio-cli/src/runner.rs +++ b/crates/nuncio-cli/src/runner.rs @@ -663,7 +663,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected sync: {status}"), + &format!( + "nunciod daemon rejected sync: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -701,7 +704,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected list_messages: {status}"), + &format!( + "nunciod daemon rejected list_messages: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -757,7 +763,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected send_message: {status}"), + &format!( + "nunciod daemon rejected send_message: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -800,7 +809,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected search_messages: {status}"), + &format!( + "nunciod daemon rejected search_messages: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -838,7 +850,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected list_folders: {status}"), + &format!( + "nunciod daemon rejected list_folders: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -877,7 +892,10 @@ impl HeadlessRunner { Self::render_error(&format!("message '{}' not found", id), json_mode) } Err(status) => Self::render_error( - &format!("nunciod daemon rejected get_message: {status}"), + &format!( + "nunciod daemon rejected get_message: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -930,7 +948,10 @@ impl HeadlessRunner { Self::render_error(&format!("message '{}' not found", id), json_mode) } Err(status) => Self::render_error( - &format!("nunciod daemon rejected mark_read: {status}"), + &format!( + "nunciod daemon rejected mark_read: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -997,7 +1018,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected export_mailbox: {status}"), + &format!( + "nunciod daemon rejected export_mailbox: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -1055,7 +1079,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected list_records: {status}"), + &format!( + "nunciod daemon rejected list_records: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -1096,7 +1123,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected verify_chain: {status}"), + &format!( + "nunciod daemon rejected verify_chain: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -1155,7 +1185,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected list_rules: {status}"), + &format!( + "nunciod daemon rejected list_rules: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -1204,7 +1237,10 @@ impl HeadlessRunner { Self::render_error(status.message(), json_mode) } Err(status) => Self::render_error( - &format!("nunciod daemon rejected create_rule: {status}"), + &format!( + "nunciod daemon rejected create_rule: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -1232,7 +1268,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected delete_rule: {status}"), + &format!( + "nunciod daemon rejected delete_rule: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -1267,7 +1306,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected validate_rule: {status}"), + &format!( + "nunciod daemon rejected validate_rule: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -1320,7 +1362,10 @@ impl HeadlessRunner { Self::render_error(status.message(), json_mode) } Err(status) => Self::render_error( - &format!("nunciod daemon rejected preview_rule: {status}"), + &format!( + "nunciod daemon rejected preview_rule: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -1372,7 +1417,10 @@ impl HeadlessRunner { Self::render_error(status.message(), json_mode) } Err(status) => Self::render_error( - &format!("nunciod daemon rejected update_rule: {status}"), + &format!( + "nunciod daemon rejected update_rule: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -1403,7 +1451,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected export_rules: {status}"), + &format!( + "nunciod daemon rejected export_rules: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -1454,7 +1505,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected import_rules: {status}"), + &format!( + "nunciod daemon rejected import_rules: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -1498,7 +1552,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected get_execution_logs: {status}"), + &format!( + "nunciod daemon rejected get_execution_logs: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -1530,7 +1587,10 @@ impl HeadlessRunner { Ok(response) => response.into_inner(), Err(status) => { return Self::render_error( - &format!("nunciod daemon rejected triage: {status}"), + &format!( + "nunciod daemon rejected triage: {}", + Self::clean_status_message(&status) + ), json_mode, ) } @@ -1559,7 +1619,10 @@ impl HeadlessRunner { Ok(None) => break, Err(status) => { return Self::render_error( - &format!("nunciod daemon triage stream failed: {status}"), + &format!( + "nunciod daemon triage stream failed: {}", + Self::clean_status_message(&status) + ), json_mode, ) } @@ -1731,7 +1794,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected add_account: {status}"), + &format!( + "nunciod daemon rejected add_account: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -1806,7 +1872,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected list_accounts: {status}"), + &format!( + "nunciod daemon rejected list_accounts: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -1825,7 +1894,12 @@ impl HeadlessRunner { let accounts = client .list_accounts(nuncio_proto::v1::ListAccountsRequest {}) .await - .map_err(|status| format!("nunciod daemon rejected list_accounts: {status}"))? + .map_err(|status| { + format!( + "nunciod daemon rejected list_accounts: {}", + Self::clean_status_message(&status) + ) + })? .into_inner() .accounts; accounts @@ -1947,7 +2021,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected update_account: {status}"), + &format!( + "nunciod daemon rejected update_account: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -1975,7 +2052,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected remove_account: {status}"), + &format!( + "nunciod daemon rejected remove_account: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -2029,7 +2109,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected test_account_connection: {status}"), + &format!( + "nunciod daemon rejected test_account_connection: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -2127,7 +2210,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected list_events: {status}"), + &format!( + "nunciod daemon rejected list_events: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -2172,7 +2258,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected calendar sync: {status}"), + &format!( + "nunciod daemon rejected calendar sync: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -2225,7 +2314,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected list_contacts: {status}"), + &format!( + "nunciod daemon rejected list_contacts: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -2271,7 +2363,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected list_contacts: {status}"), + &format!( + "nunciod daemon rejected list_contacts: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -2329,7 +2424,10 @@ impl HeadlessRunner { ), }, Err(status) => Self::render_error( - &format!("nunciod daemon rejected create_contact: {status}"), + &format!( + "nunciod daemon rejected create_contact: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -2363,7 +2461,10 @@ impl HeadlessRunner { } } Err(status) => Self::render_error( - &format!("nunciod daemon rejected contacts sync: {status}"), + &format!( + "nunciod daemon rejected contacts sync: {}", + Self::clean_status_message(&status) + ), json_mode, ), } @@ -2379,6 +2480,22 @@ impl HeadlessRunner { } } + /// Extracts the clean, user-facing text from a `tonic::Status` returned + /// by a daemon RPC. Uses the server-provided message when present; + /// falls back to a short " error" form when the message is empty. + /// Callers MUST use this (never the `Display`/`Debug` of `tonic::Status` + /// directly) for user-facing output, since `Display`/`Debug` on + /// `tonic::Status` dumps raw gRPC internals (`status: ..., metadata: + /// MetadataMap { ... }`) that are meaningless to a CLI user. + fn clean_status_message(status: &tonic::Status) -> String { + let message = status.message(); + if message.is_empty() { + format!("{:?} error", status.code()) + } else { + message.to_string() + } + } + /// `system status`: a real thin gRPC client of the running `nunciod` /// daemon's `nuncio.v1.System` API. /// @@ -2431,7 +2548,12 @@ impl HeadlessRunner { let response = client .get_status(nuncio_proto::v1::GetStatusRequest {}) .await - .map_err(|e| format!("nunciod daemon rejected status request: {e}"))? + .map_err(|status| { + format!( + "nunciod daemon rejected status request: {}", + Self::clean_status_message(&status) + ) + })? .into_inner(); Ok((response.engine_status, response.version)) @@ -3349,6 +3471,109 @@ mod tests { ); } + /// Proves that a generic (non-`NotFound`/`InvalidArgument`) RPC failure + /// is rendered as the server-provided `status.message()` text, never + /// the raw `Display`/`Debug` dump of `tonic::Status` (which leaks + /// internal gRPC framing like `metadata: MetadataMap { ... }` into + /// CLI output). + #[tokio::test] + async fn mail_rpc_error_renders_clean_message_not_raw_status_dump() { + use nuncio_proto::v1::mail_server::{Mail as MailService, MailServer}; + use nuncio_proto::v1::{ + GetMessageRequest, GetMessageResponse, ListFoldersRequest, ListFoldersResponse, + ListMessagesRequest, ListMessagesResponse, MarkReadRequest, MarkReadResponse, + SearchMessagesRequest, SearchMessagesResponse, SendMessageRequest, SendMessageResponse, + SyncRequest, SyncResponse, + }; + + /// Minimal test-only stub of `nuncio.v1.Mail` whose `ListFolders` + /// always fails with an internal error, so this test can assert + /// on exactly how the CLI renders a generic RPC failure. + struct StubMailAlwaysErrors; + + #[tonic::async_trait] + impl MailService for StubMailAlwaysErrors { + async fn list_folders( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Err(tonic::Status::internal("mailbox index is corrupt")) + } + + async fn list_messages( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Err(tonic::Status::internal("mailbox index is corrupt")) + } + + async fn get_message( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Err(tonic::Status::internal("mailbox index is corrupt")) + } + + async fn mark_read( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Err(tonic::Status::internal("mailbox index is corrupt")) + } + + async fn search_messages( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Err(tonic::Status::internal("mailbox index is corrupt")) + } + + async fn send_message( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Err(tonic::Status::internal("mailbox index is corrupt")) + } + + async fn sync( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Err(tonic::Status::internal("mailbox index is corrupt")) + } + } + + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind ephemeral loopback port"); + let addr = listener.local_addr().expect("listener has local addr"); + tokio::spawn(async move { + let _ = tonic::transport::Server::builder() + .add_service(MailServer::new(StubMailAlwaysErrors)) + .serve_with_incoming(tokio_stream::wrappers::TcpListenerStream::new(listener)) + .await; + }); + + let runner = + HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr.to_string()) + .await + .expect("ephemeral runner initializes"); + + let out = runner + .execute_command( + &Commands::Folder { + action: FolderSubcommand::List, + }, + false, + ) + .await; + + assert!(out.contains("mailbox index is corrupt")); + assert!(!out.contains("metadata:")); + assert!(!out.contains("MetadataMap")); + assert!(!out.contains("status:")); + } + /// Reference-client proof: boots a stub `nuncio.v1.Calendar` gRPC server /// (mirroring the `Mail` stub pattern above) and drives the real /// `HeadlessRunner`'s `cal list`/`cal sync` gRPC client paths against it. -- 2.54.0 From 546cebe7230edb9ace29362247e2fd4b0d68c698 Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 31 Jul 2026 13:35:15 -0500 Subject: [PATCH 2/4] fix(core): reject cleartext Plain TLS to non-loopback hosts (#335) * fix(core): reject cleartext Plain TLS to non-loopback hosts AccountConfig::validate silently accepted TlsMode::Plain for imap/smtp against any host, so a misconfig could send credentials in cleartext to a remote server. Plain is now rejected unless the host is loopback (127.0.0.1, ::1, or localhost); ImplicitTls/StartTls are unaffected. Refs #297 * fix(core): reject cleartext CalDAV collection_url on remote hosts The CalDAV collection_url validation only checked the scheme prefix, so http:///... passed even though it sends the account's Basic-auth credentials in cleartext. Reuse the same loopback-only rule as the imap/smtp TlsMode::Plain check: plain http is now accepted only when the URL's host is loopback (127.0.0.0/8, ::1, or localhost); https to any host, and http to a loopback host, remain valid. Refs #297 --------- Co-authored-by: James Maes --- crates/nuncio-core/src/config.rs | 147 ++++++++++++++++++ .../tests/accounts_lifecycle_e2e_test.rs | 2 +- 2 files changed, 148 insertions(+), 1 deletion(-) diff --git a/crates/nuncio-core/src/config.rs b/crates/nuncio-core/src/config.rs index 5cbb012..df39c9c 100644 --- a/crates/nuncio-core/src/config.rs +++ b/crates/nuncio-core/src/config.rs @@ -61,6 +61,17 @@ pub enum ConfigError { /// A DAV-protocol account's collection URL is not a valid http(s) URL. #[error("collection_url must be a valid http(s) URL")] InvalidCollectionUrl, + /// Cleartext `TlsMode::Plain` was requested against a non-loopback host, + /// which would send credentials over the network unencrypted. + #[error( + "{endpoint} host '{host}' is not loopback; refusing cleartext Plain TLS mode to a remote host" + )] + CleartextToRemoteHost { + /// Which endpoint the rejected setting applies to (e.g. "imap", "smtp"). + endpoint: &'static str, + /// The offending non-loopback host. + host: String, + }, } /// Complete configuration schema for a mail & calendar account. @@ -102,6 +113,35 @@ pub struct AccountConfig { pub collection_url: String, } +/// Whether `host` refers to the local loopback interface, either as a +/// literal IP address (`127.0.0.1`, `::1`, ...) or the conventional +/// `localhost` name. Used to allow cleartext `TlsMode::Plain` only against +/// local test servers, never against a remote host. +fn is_loopback_host(host: &str) -> bool { + let host = host.trim(); + if host.eq_ignore_ascii_case("localhost") { + return true; + } + host.parse::() + .is_ok_and(|ip| ip.is_loopback()) +} + +/// Extract the host portion of an `http://`/`https://` URL, without pulling +/// in a full URL-parsing dependency for this one field. Handles a bracketed +/// IPv6 literal (`[::1]`) as well as a plain hostname/IPv4 literal, and +/// stops at the first `:` (port), `/` (path), `?`, or `#` that follows. +fn url_host(url: &str) -> Option<&str> { + let rest = url + .strip_prefix("https://") + .or_else(|| url.strip_prefix("http://"))?; + if let Some(bracketed) = rest.strip_prefix('[') { + let end = bracketed.find(']')?; + return Some(&bracketed[..end]); + } + let end = rest.find(['/', ':', '?', '#']).unwrap_or(rest.len()); + Some(&rest[..end]) +} + impl AccountConfig { /// Minimum allowed background sync interval (10 seconds). pub const MIN_SYNC_INTERVAL_SECS: u64 = 10; @@ -140,6 +180,18 @@ impl AccountConfig { if !(url.starts_with("http://") || url.starts_with("https://")) { return Err(ConfigError::InvalidCollectionUrl); } + // Plain `http://` is only tolerated against a loopback host (a + // local test server); any remote host must use `https://` to + // avoid sending the account's Basic-auth credentials in cleartext. + if url.starts_with("http://") { + let host = url_host(url).unwrap_or_default(); + if !is_loopback_host(host) { + return Err(ConfigError::CleartextToRemoteHost { + endpoint: "caldav", + host: host.to_string(), + }); + } + } } else { if self.server_host.trim().is_empty() { return Err(ConfigError::EmptyField { @@ -155,6 +207,18 @@ impl AccountConfig { if self.smtp_port == 0 { return Err(ConfigError::InvalidPort); } + if self.imap_tls_mode == TlsMode::Plain && !is_loopback_host(&self.server_host) { + return Err(ConfigError::CleartextToRemoteHost { + endpoint: "imap", + host: self.server_host.clone(), + }); + } + if self.smtp_tls_mode == TlsMode::Plain && !is_loopback_host(&self.smtp_host) { + return Err(ConfigError::CleartextToRemoteHost { + endpoint: "smtp", + host: self.smtp_host.clone(), + }); + } } if self.keyring_secret_key.trim().is_empty() { return Err(ConfigError::EmptyField { @@ -239,6 +303,35 @@ mod tests { ); } + #[test] + fn caldav_account_rejects_cleartext_http_to_remote_host() { + let mut config = valid_caldav_account(); + config.collection_url = "http://remote/dav/".to_string(); + assert_eq!( + config.validate().unwrap_err(), + ConfigError::CleartextToRemoteHost { + endpoint: "caldav", + host: "remote".to_string(), + } + ); + } + + #[test] + fn caldav_account_accepts_https_to_remote_host() { + let mut config = valid_caldav_account(); + config.collection_url = "https://remote/dav/".to_string(); + assert!(config.validate().is_ok()); + } + + #[test] + fn caldav_account_accepts_cleartext_http_to_loopback_host() { + for url in ["http://127.0.0.1/dav/", "http://localhost/dav/"] { + let mut config = valid_caldav_account(); + config.collection_url = url.to_string(); + assert!(config.validate().is_ok(), "url {url} should be allowed"); + } + } + #[test] fn account_protocol_is_dav_only_for_dav_protocols() { assert!(AccountProtocol::CalDav.is_dav()); @@ -358,6 +451,52 @@ mod tests { ); } + #[test] + fn plain_imap_to_remote_host_fails_validation() { + let mut config = valid_account(); + config.imap_tls_mode = TlsMode::Plain; + assert_eq!( + config.validate().unwrap_err(), + ConfigError::CleartextToRemoteHost { + endpoint: "imap", + host: "jmap.nuncio.mx".to_string(), + } + ); + } + + #[test] + fn plain_smtp_to_remote_host_fails_validation() { + let mut config = valid_account(); + config.smtp_tls_mode = TlsMode::Plain; + assert_eq!( + config.validate().unwrap_err(), + ConfigError::CleartextToRemoteHost { + endpoint: "smtp", + host: "smtp.nuncio.mx".to_string(), + } + ); + } + + #[test] + fn plain_imap_and_smtp_to_loopback_hosts_pass_validation() { + for host in ["127.0.0.1", "localhost", "::1", "LOCALHOST"] { + let mut config = valid_account(); + config.server_host = host.to_string(); + config.smtp_host = host.to_string(); + config.imap_tls_mode = TlsMode::Plain; + config.smtp_tls_mode = TlsMode::Plain; + assert!(config.validate().is_ok(), "host {host} should be allowed"); + } + } + + #[test] + fn implicit_tls_and_starttls_to_remote_host_pass_validation() { + let mut config = valid_account(); + config.imap_tls_mode = TlsMode::ImplicitTls; + config.smtp_tls_mode = TlsMode::StartTls; + assert!(config.validate().is_ok()); + } + #[test] fn account_protocol_serde_roundtrip() { let jmap_json = serde_json::to_string(&AccountProtocol::Jmap).unwrap(); @@ -415,5 +554,13 @@ mod tests { ConfigError::SyncIntervalTooShort.to_string(), "sync interval must be at least 10 seconds" ); + assert_eq!( + ConfigError::CleartextToRemoteHost { + endpoint: "imap", + host: "mail.example.com".to_string(), + } + .to_string(), + "imap host 'mail.example.com' is not loopback; refusing cleartext Plain TLS mode to a remote host" + ); } } diff --git a/crates/nunciod/tests/accounts_lifecycle_e2e_test.rs b/crates/nunciod/tests/accounts_lifecycle_e2e_test.rs index a6dd608..10c5b42 100644 --- a/crates/nunciod/tests/accounts_lifecycle_e2e_test.rs +++ b/crates/nunciod/tests/accounts_lifecycle_e2e_test.rs @@ -87,7 +87,7 @@ fn sample_account_config() -> AccountConfig { smtp_tls_mode: TlsMode::Plain.into(), keyring_secret_key: KEYRING_KEY.to_string(), sync_interval_secs: 60, - smtp_host: "smtp.nuncio.mx".to_string(), + smtp_host: "127.0.0.1".to_string(), smtp_port: 25, collection_url: String::new(), } -- 2.54.0 From 674a158d080569d663cd20c33fd1068b98c09f21 Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 31 Jul 2026 13:42:58 -0500 Subject: [PATCH 3/4] fix(store): give messages a deterministic surrogate identity IMAP UIDs are unique only per mailbox+UIDVALIDITY, yet messages.id was the sole key and Email.id was "imap-uid-{uid}", so the same UID in two folders or accounts collapsed to one row and silently overwrote (audit C1). The id shape also leaked internal encoding on the wire. Messages now carry an opaque surrogate id = SHA-256 over length-prefixed (account, folder, uid_validity, remote_id), plus first-class remote_id and uid_validity columns and a UNIQUE(account, folder, uid_validity, remote_id) index (backfill-safe migration seeding legacy rows from the old id). The IMAP and JMAP backends populate remote_id/uid_validity and hash the id; the outbox executor recovers remote addressing from the columns instead of parsing the id, still SELECT-verifying UIDVALIDITY before mutating. Re-sync recomputes the same id, so it upserts one row and the fire-once filter guard still holds. Message.id stays a proto string; only its value becomes opaque (no proto change). Refs #281 --- crates/nuncio-core/src/export.rs | 4 + crates/nuncio-core/src/model.rs | 81 +++++++- crates/nuncio-filter/src/engine.rs | 4 + crates/nuncio-mail/src/backend.rs | 32 ++-- crates/nuncio-mail/src/imap.rs | 96 ++++++---- crates/nuncio-mail/src/jmap.rs | 36 +++- crates/nuncio-mail/src/mock.rs | 2 + crates/nuncio-mail/src/parser.rs | 4 + crates/nuncio-mail/src/smtp.rs | 2 + .../nuncio-mail/tests/wiremock_jmap_test.rs | 15 +- crates/nuncio-store/src/db.rs | 181 +++++++++++++++++- crates/nuncio-store/src/search.rs | 2 + crates/nunciod/src/grpc.rs | 15 ++ crates/nunciod/src/outbox.rs | 23 +-- crates/nunciod/src/sync.rs | 11 +- .../nunciod/tests/filters_triage_e2e_test.rs | 2 + .../nunciod/tests/outbox_executor_e2e_test.rs | 14 +- crates/nunciod/tests/spine_e2e_test.rs | 2 + 18 files changed, 434 insertions(+), 92 deletions(-) diff --git a/crates/nuncio-core/src/export.rs b/crates/nuncio-core/src/export.rs index 360773f..d180291 100644 --- a/crates/nuncio-core/src/export.rs +++ b/crates/nuncio-core/src/export.rs @@ -185,6 +185,8 @@ mod tests { id: "msg-100".to_string(), account_id: "acct-1".to_string(), folder_id: "INBOX".to_string(), + remote_id: "100".to_string(), + uid_validity: "1".to_string(), subject: "Test Subject 1".to_string(), sender: "alice@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), @@ -198,6 +200,8 @@ mod tests { id: "msg-101".to_string(), account_id: "acct-1".to_string(), folder_id: "INBOX".to_string(), + remote_id: "101".to_string(), + uid_validity: "1".to_string(), subject: "Test Subject 2".to_string(), sender: "charlie@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), diff --git a/crates/nuncio-core/src/model.rs b/crates/nuncio-core/src/model.rs index 311c1b5..b6e5034 100644 --- a/crates/nuncio-core/src/model.rs +++ b/crates/nuncio-core/src/model.rs @@ -2,6 +2,7 @@ use bytes::Bytes; use serde::{Deserialize, Serialize}; +use sha2::{Digest, Sha256}; /// Real-time daemon health, performance metrics, and telemetry payload. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -63,12 +64,30 @@ pub struct Attachment { /// Email message domain entity owned by Nuncio core. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct Email { - /// Unique message identifier. + /// Opaque, deterministic surrogate identifier for the message. + /// + /// Derived from [`Email::surrogate_id`] over the account, folder, + /// UIDVALIDITY, and protocol-native id. It carries no decodable internal + /// encoding (callers must treat it as opaque) and is stable across + /// re-syncs of the same message, so persisting it upserts the same row + /// rather than duplicating. Distinct messages that happen to share a + /// protocol id across folders or accounts hash to distinct surrogates, so + /// one can never silently overwrite another. pub id: String, /// Account identifier owning the message. pub account_id: String, /// Mailbox folder identifier (e.g. "inbox"). pub folder_id: String, + /// Protocol-native message id used to address the message on its server: + /// the IMAP UID as a decimal string, or the JMAP Email object id. This is + /// what a remote mutation must use to identify the message on the wire -- + /// never the opaque [`Email::id`]. + pub remote_id: String, + /// The addressing scope the `remote_id` is only meaningful within: the IMAP + /// folder UIDVALIDITY as a decimal string. Protocols without a UIDVALIDITY + /// (JMAP) carry a stable sentinel instead, so the surrogate id stays + /// deterministic. + pub uid_validity: String, /// Subject line. pub subject: String, /// Sender address (e.g. "alice@nuncio.mx"). @@ -87,6 +106,33 @@ pub struct Email { pub attachments: Vec, } +impl Email { + /// Compute the deterministic, collision-resistant surrogate id for a + /// message from its addressing coordinates. + /// + /// The four coordinates together uniquely identify a stored message: a + /// protocol-native `remote_id` (IMAP UID / JMAP object id) is only unique + /// within one `uid_validity` scope of one `folder_id` of one `account_id`, + /// so all four must participate. Each field is fed length-prefixed so that + /// no two distinct coordinate tuples can serialize to the same byte stream + /// (e.g. `("a", "bc")` and `("ab", "c")` must not collide). The SHA-256 + /// digest is hex-encoded, yielding a stable opaque token that leaks none of + /// its inputs' encoding on the wire. + pub fn surrogate_id( + account_id: &str, + folder_id: &str, + uid_validity: &str, + remote_id: &str, + ) -> String { + let mut hasher = Sha256::new(); + for field in [account_id, folder_id, uid_validity, remote_id] { + hasher.update((field.len() as u64).to_le_bytes()); + hasher.update(field.as_bytes()); + } + hex::encode(hasher.finalize()) + } +} + /// Mailbox folder entity. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct Folder { @@ -168,4 +214,37 @@ mod tests { assert_eq!(parsed.status, "HEALTHY"); assert_eq!(parsed.total_emails, 15000); } + + #[test] + fn surrogate_id_is_deterministic_and_opaque() { + let a = Email::surrogate_id("acct-1", "INBOX", "42", "5"); + let b = Email::surrogate_id("acct-1", "INBOX", "42", "5"); + assert_eq!(a, b, "identical coordinates must hash identically"); + // A 32-byte SHA-256 digest hex-encodes to 64 chars, and the token must + // not leak the protocol-native id it was built from. + assert_eq!(a.len(), 64); + assert!(a.chars().all(|c| c.is_ascii_hexdigit())); + assert!(!a.contains("imap-uid-")); + } + + #[test] + fn surrogate_id_distinguishes_the_cross_folder_and_cross_account_collision() { + // The same protocol UID (5) in two different folders of one account, or + // in two different accounts, must NOT collapse to the same surrogate. + let inbox = Email::surrogate_id("acct-1", "INBOX", "42", "5"); + let sent = Email::surrogate_id("acct-1", "Sent", "7", "5"); + let other_account = Email::surrogate_id("acct-2", "INBOX", "42", "5"); + assert_ne!(inbox, sent, "same UID in two folders must not collide"); + assert_ne!( + inbox, other_account, + "same UID in two accounts must not collide" + ); + + // Field boundaries are unambiguous: shifting a delimiter-like split + // across two adjacent fields yields a different id. + assert_ne!( + Email::surrogate_id("a", "bc", "1", "1"), + Email::surrogate_id("ab", "c", "1", "1"), + ); + } } diff --git a/crates/nuncio-filter/src/engine.rs b/crates/nuncio-filter/src/engine.rs index bce77e8..6037db1 100644 --- a/crates/nuncio-filter/src/engine.rs +++ b/crates/nuncio-filter/src/engine.rs @@ -373,6 +373,8 @@ mod tests { id: "msg-1".to_string(), account_id: account_id.to_string(), folder_id: folder_id.to_string(), + remote_id: "1".to_string(), + uid_validity: "1".to_string(), subject: subject.to_string(), sender: "alice@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), @@ -394,6 +396,8 @@ mod tests { id: "msg-1".to_string(), account_id: "acct-1".to_string(), folder_id: "inbox".to_string(), + remote_id: "1".to_string(), + uid_validity: "1".to_string(), subject: "Urgent Meeting".to_string(), sender: "alice@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), diff --git a/crates/nuncio-mail/src/backend.rs b/crates/nuncio-mail/src/backend.rs index 5c9bb96..cb999bd 100644 --- a/crates/nuncio-mail/src/backend.rs +++ b/crates/nuncio-mail/src/backend.rs @@ -36,25 +36,31 @@ pub enum RemoteMutationKind { Delete, } -/// A fully-addressed remote mutation for a single message: which message -/// (protocol-native id), the folder it currently lives in, that folder's -/// stored sync checkpoint (carrying IMAP UIDVALIDITY), and the change to -/// apply. +/// A fully-addressed remote mutation for a single message: the protocol-native +/// id to address it on the server, the folder it currently lives in, the +/// UIDVALIDITY scope that id was captured under, and the change to apply. /// -/// `folder_checkpoint` is the value persisted by a prior sync -/// (`"{uidvalidity}:{uidnext}"` for IMAP). The IMAP backend uses it to refuse -/// to act when the mailbox's current UIDVALIDITY no longer matches -- a -/// renumbered mailbox reassigns UIDs, so the stored UID would otherwise -/// address the wrong message. Protocols that do not use UIDVALIDITY (JMAP) -/// ignore it. +/// Addressing is driven by `remote_id` (the IMAP UID / JMAP object id), NOT by +/// the opaque `message_id` -- so remote-mutation targeting never depends on the +/// shape of the surrogate id. `uid_validity` is the mailbox UIDVALIDITY the +/// `remote_id` was captured under; the IMAP backend refuses to act unless the +/// mailbox's current UIDVALIDITY still matches it, because a renumbered mailbox +/// (RFC 3501 s2.3.1.1) reassigns UIDs and the stored UID would otherwise +/// address the wrong message. Protocols without a UIDVALIDITY (JMAP) carry a +/// stable sentinel and ignore this guard. #[derive(Debug, Clone, PartialEq, Eq)] pub struct RemoteMutationSpec { - /// Protocol-native message id (IMAP `imap-uid-{uid}`, JMAP object id). + /// Opaque surrogate id of the target message. Diagnostic only -- it names + /// the message in errors/logs and is never used to address it on the wire. pub message_id: String, + /// Protocol-native id used to address the message on its server: the IMAP + /// UID as a decimal string, or the JMAP Email object id. + pub remote_id: String, /// Folder the message currently lives in. pub folder_id: String, - /// The source folder's stored sync checkpoint, if any. - pub folder_checkpoint: Option, + /// The UIDVALIDITY the `remote_id` was captured under (IMAP; decimal + /// string), or a protocol sentinel for servers without UIDVALIDITY (JMAP). + pub uid_validity: String, /// The change to apply. pub kind: RemoteMutationKind, } diff --git a/crates/nuncio-mail/src/imap.rs b/crates/nuncio-mail/src/imap.rs index adfbf63..c5288ac 100644 --- a/crates/nuncio-mail/src/imap.rs +++ b/crates/nuncio-mail/src/imap.rs @@ -34,31 +34,21 @@ const FETCH_ITEM_TIMEOUT: Duration = Duration::from_secs(60); use crate::backend::{MailBackend, RemoteMutationKind, RemoteMutationSpec}; use crate::parser::{MailError, MimeParserAdapter}; -/// Prefix an IMAP [`Email::id`] carries in front of its raw numeric UID (see -/// [`ImapEngine::sync_folder_messages_with_session`], which formats every id as -/// `"imap-uid-{uid}"`). A remote mutation must recover the UID from the id to -/// address the message, so the prefix is defined once here. -const IMAP_UID_ID_PREFIX: &str = "imap-uid-"; - -/// Recover the raw IMAP UID from an [`Email::id`] of the form -/// `"imap-uid-{uid}"`. Returns `None` for any id that does not carry the -/// expected prefix and a valid non-zero `u32` UID, so a mutation against a -/// malformed/foreign id fails honestly rather than addressing UID 0. -fn parse_imap_uid(message_id: &str) -> Option { - let raw = message_id.strip_prefix(IMAP_UID_ID_PREFIX)?; - let uid = raw.trim().parse::().ok()?; +/// Recover a raw IMAP UID from a [`RemoteMutationSpec::remote_id`] (a decimal +/// string). Returns `None` for anything that is not a valid non-zero `u32`, so +/// a mutation against a malformed/foreign remote id fails honestly rather than +/// addressing UID 0. +fn parse_imap_uid(remote_id: &str) -> Option { + let uid = remote_id.trim().parse::().ok()?; (uid >= 1).then_some(uid) } -/// Extract the UIDVALIDITY component of a stored folder checkpoint -/// (`"{uidvalidity}:{uidnext}"`). Returns `None` for a missing, malformed, or -/// legacy bare-number checkpoint -- the caller treats an absent stored -/// UIDVALIDITY as "cannot prove the mailbox has not been renumbered" and -/// refuses to act. -fn checkpoint_uid_validity(checkpoint: Option<&str>) -> Option { - let raw = checkpoint?; - let (validity, _uid) = raw.split_once(CHECKPOINT_DELIM)?; - validity.trim().parse::().ok() +/// Parse a stored [`RemoteMutationSpec::uid_validity`] (a decimal string) into +/// a `u32`. Returns `None` for a missing or non-numeric value -- the caller +/// treats an unparseable UIDVALIDITY as "cannot prove the mailbox has not been +/// renumbered" and refuses to act. +fn parse_uid_validity(uid_validity: &str) -> Option { + uid_validity.trim().parse::().ok() } /// Delimiter between the two components of a folder sync checkpoint. The @@ -652,7 +642,16 @@ impl ImapEngine { let uid_num = fetch_data.uid.unwrap_or(0); max_uid_seen = max_uid_seen.max(uid_num); - let email_id = format!("imap-uid-{}", uid_num); + // The message is addressed on the wire by its UID within the + // folder's UIDVALIDITY scope; the persisted id is an opaque + // surrogate hashed over both plus the account and folder, so the + // same UID in another folder/account can never collide. + let remote_id = uid_num.to_string(); + let uid_validity = server_uid_validity + .map(|v| v.to_string()) + .unwrap_or_default(); + let email_id = + Email::surrogate_id(&self.account_id, folder_id, &uid_validity, &remote_id); let is_read = fetch_data .flags() @@ -665,6 +664,8 @@ impl ImapEngine { folder_id, raw_bytes, )?; + email.remote_id = remote_id; + email.uid_validity = uid_validity; email.read = is_read; email } else { @@ -721,6 +722,8 @@ impl ImapEngine { id: email_id, account_id: self.account_id.clone(), folder_id: folder_id.to_string(), + remote_id, + uid_validity, subject, sender, recipient, @@ -815,10 +818,10 @@ impl ImapEngine { where S: AsyncRead + AsyncWrite + Unpin + Send + Debug + 'static, { - let uid = parse_imap_uid(&spec.message_id).ok_or_else(|| { + let uid = parse_imap_uid(&spec.remote_id).ok_or_else(|| { MailError::ImapError(format!( - "cannot parse an IMAP UID from message id '{}'", - spec.message_id + "cannot parse an IMAP UID from remote id '{}' (message '{}')", + spec.remote_id, spec.message_id )) })?; @@ -831,7 +834,7 @@ impl ImapEngine { // Refuse to act unless the mailbox's current UIDVALIDITY provably // matches the one the stored UID was captured under. - let stored_validity = checkpoint_uid_validity(spec.folder_checkpoint.as_deref()); + let stored_validity = parse_uid_validity(&spec.uid_validity); match (stored_validity, mailbox.uid_validity) { (Some(stored), Some(current)) if stored == current => {} (stored, current) => { @@ -1575,23 +1578,24 @@ mod tests { } #[test] - fn parse_imap_uid_recovers_uid_and_rejects_malformed_ids() { - assert_eq!(parse_imap_uid("imap-uid-42"), Some(42)); - assert_eq!(parse_imap_uid("imap-uid-1"), Some(1)); - // UID 0 is not a valid IMAP UID; a bare/foreign id yields nothing. - assert_eq!(parse_imap_uid("imap-uid-0"), None); - assert_eq!(parse_imap_uid("42"), None); + fn parse_imap_uid_recovers_uid_and_rejects_non_numeric_ids() { + assert_eq!(parse_imap_uid("42"), Some(42)); + assert_eq!(parse_imap_uid(" 42 "), Some(42)); + assert_eq!(parse_imap_uid("1"), Some(1)); + // UID 0 is not a valid IMAP UID; a non-numeric/foreign id yields nothing. + assert_eq!(parse_imap_uid("0"), None); + assert_eq!(parse_imap_uid("imap-uid-42"), None); assert_eq!(parse_imap_uid("jmap-object-id"), None); } #[test] - fn checkpoint_uid_validity_extracts_only_a_well_formed_validity() { - assert_eq!(checkpoint_uid_validity(Some("7:105")), Some(7)); - assert_eq!(checkpoint_uid_validity(Some(" 7 : 105 ")), Some(7)); - assert_eq!(checkpoint_uid_validity(None), None); - // A legacy bare-number checkpoint carries no validity to compare. - assert_eq!(checkpoint_uid_validity(Some("105")), None); - assert_eq!(checkpoint_uid_validity(Some("x:105")), None); + fn parse_uid_validity_accepts_only_a_decimal_value() { + assert_eq!(parse_uid_validity("7"), Some(7)); + assert_eq!(parse_uid_validity(" 7 "), Some(7)); + assert_eq!(parse_uid_validity(""), None); + // The JMAP sentinel and a full checkpoint string are not a bare validity. + assert_eq!(parse_uid_validity("jmap"), None); + assert_eq!(parse_uid_validity("7:105"), None); } /// Drive a mutation over a scripted IMAP server (in-memory duplex), @@ -1697,10 +1701,18 @@ mod tests { } fn imap_spec(kind: RemoteMutationKind, checkpoint: Option<&str>) -> RemoteMutationSpec { + // The tests express the captured scope as a "{uidvalidity}:{uidnext}" + // checkpoint string (mirroring how a sync stores it); the spec now + // carries just the UIDVALIDITY component the guard compares against. + let uid_validity = checkpoint + .and_then(|c| c.split(':').next()) + .unwrap_or_default() + .to_string(); RemoteMutationSpec { - message_id: "imap-uid-42".to_string(), + message_id: "surrogate-42".to_string(), + remote_id: "42".to_string(), folder_id: "INBOX".to_string(), - folder_checkpoint: checkpoint.map(str::to_string), + uid_validity, kind, } } diff --git a/crates/nuncio-mail/src/jmap.rs b/crates/nuncio-mail/src/jmap.rs index b7a1e0a..cfbc0fa 100644 --- a/crates/nuncio-mail/src/jmap.rs +++ b/crates/nuncio-mail/src/jmap.rs @@ -8,6 +8,14 @@ use serde_json::{json, Value}; use crate::backend::{MailBackend, RemoteMutationKind, RemoteMutationSpec}; use crate::parser::MailError; +/// Stand-in for the UIDVALIDITY scope on JMAP, which has no such concept: a +/// JMAP Email object id is already globally stable within its account (RFC +/// 8621), so a fixed sentinel keeps the surrogate-id computation uniform with +/// IMAP without implying a UIDVALIDITY guard. The IMAP UIDVALIDITY guard treats +/// this non-numeric value as "no validity to compare" and is never engaged for +/// a JMAP mutation, which addresses messages by object id. +const JMAP_UID_VALIDITY_SENTINEL: &str = "jmap"; + /// JMAP Session Object (RFC 8620 Section 2). #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -214,7 +222,9 @@ impl JmapEngine { /// `$flagged` keyword (`true` to set, `null` to clear), the JMAP equivalent /// of IMAP `\Flagged`. pub fn build_email_set_request(account_id: &str, spec: &RemoteMutationSpec) -> Value { - let id = &spec.message_id; + // Address the message by its protocol-native JMAP object id, never the + // opaque surrogate `message_id`. + let id = &spec.remote_id; let (update, destroy) = match &spec.kind { RemoteMutationKind::SetFlagged { value } => { let keyword = if *value { json!(true) } else { Value::Null }; @@ -383,10 +393,23 @@ impl JmapEngine { .map(|a| a.email.clone()) .unwrap_or_else(|| "me@nuncio.mx".to_string()); + let folder_id = "inbox".to_string(); + // The JMAP object id is the protocol-native id; the persisted + // id is an opaque surrogate hashed over it plus the account, + // folder, and (sentinel) scope so it stays uniform with IMAP. + let remote_id = item.id; + let id = Email::surrogate_id( + &self.account_id, + &folder_id, + JMAP_UID_VALIDITY_SENTINEL, + &remote_id, + ); Email { - id: item.id, + id, account_id: self.account_id.clone(), - folder_id: "inbox".to_string(), + folder_id, + remote_id, + uid_validity: JMAP_UID_VALIDITY_SENTINEL.to_string(), subject: item.subject.unwrap_or_else(|| "No Subject".to_string()), sender, recipient, @@ -558,7 +581,7 @@ impl MailBackend for JmapEngine { let request = Self::build_email_set_request(&account_id, spec); let raw = self.post_jmap(&session.api_url, &request).await?; let expect_destroy = matches!(spec.kind, RemoteMutationKind::Delete); - Self::confirm_email_set_applied(&raw, &spec.message_id, expect_destroy) + Self::confirm_email_set_applied(&raw, &spec.remote_id, expect_destroy) } } @@ -689,9 +712,10 @@ mod tests { fn spec(kind: RemoteMutationKind) -> RemoteMutationSpec { RemoteMutationSpec { - message_id: "m-1".to_string(), + message_id: "surrogate-m-1".to_string(), + remote_id: "m-1".to_string(), folder_id: "mb-inbox".to_string(), - folder_checkpoint: None, + uid_validity: JMAP_UID_VALIDITY_SENTINEL.to_string(), kind, } } diff --git a/crates/nuncio-mail/src/mock.rs b/crates/nuncio-mail/src/mock.rs index 4a581d5..487d09a 100644 --- a/crates/nuncio-mail/src/mock.rs +++ b/crates/nuncio-mail/src/mock.rs @@ -244,6 +244,8 @@ mod tests { id: "msg-mock-1".to_string(), account_id: "acct-1".to_string(), folder_id: "inbox".to_string(), + remote_id: "1".to_string(), + uid_validity: "1".to_string(), subject: "Mock Test".to_string(), sender: "alice@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), diff --git a/crates/nuncio-mail/src/parser.rs b/crates/nuncio-mail/src/parser.rs index 28976b9..95e19f7 100644 --- a/crates/nuncio-mail/src/parser.rs +++ b/crates/nuncio-mail/src/parser.rs @@ -129,6 +129,10 @@ impl MimeParserAdapter { id: id.to_string(), account_id: account_id.to_string(), folder_id: folder_id.to_string(), + // The MIME body carries no protocol addressing; the caller (the + // IMAP sync) fills these from the transport before persisting. + remote_id: String::new(), + uid_validity: String::new(), subject, sender, recipient, diff --git a/crates/nuncio-mail/src/smtp.rs b/crates/nuncio-mail/src/smtp.rs index 35e592d..9cef1f0 100644 --- a/crates/nuncio-mail/src/smtp.rs +++ b/crates/nuncio-mail/src/smtp.rs @@ -329,6 +329,8 @@ mod tests { id: "msg-1".to_string(), account_id: "acct-1".to_string(), folder_id: "outbox".to_string(), + remote_id: "1".to_string(), + uid_validity: "1".to_string(), subject: "Status Update".to_string(), sender: "alice@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), diff --git a/crates/nuncio-mail/tests/wiremock_jmap_test.rs b/crates/nuncio-mail/tests/wiremock_jmap_test.rs index f7773af..f809590 100644 --- a/crates/nuncio-mail/tests/wiremock_jmap_test.rs +++ b/crates/nuncio-mail/tests/wiremock_jmap_test.rs @@ -84,7 +84,11 @@ async fn wiremock_jmap_session_discovery_and_email_get_sync() { .expect("parse email get"); assert_eq!(emails.len(), 1); - assert_eq!(emails[0].id, "msg-wm-100"); + // The JMAP object id is preserved as the protocol-native `remote_id`, while + // the persisted id is an opaque 64-char surrogate hash (never the raw id). + assert_eq!(emails[0].remote_id, "msg-wm-100"); + assert_ne!(emails[0].id, "msg-wm-100"); + assert_eq!(emails[0].id.len(), 64); assert_eq!(emails[0].subject, "WireMock JMAP E2E Test"); assert_eq!(emails[0].sender, "sender@nuncio.mx"); assert_eq!(new_state, "sync-state-500"); @@ -200,7 +204,9 @@ async fn jmap_engine_sync_folders_and_messages_through_mail_backend_trait() { .await .expect("sync_messages succeeds"); assert_eq!(emails.len(), 1); - assert_eq!(emails[0].id, "msg-wm-200"); + assert_eq!(emails[0].remote_id, "msg-wm-200"); + assert_ne!(emails[0].id, "msg-wm-200"); + assert_eq!(emails[0].id.len(), 64); assert_eq!(emails[0].subject, "Trait-Level JMAP Sync"); assert_eq!(emails[0].sender, "sender@nuncio.mx"); assert_eq!(emails[0].received_at, 1700002000); @@ -264,9 +270,10 @@ fn jmap_engine(mock_server: &MockServer) -> JmapEngine { fn spec(kind: RemoteMutationKind) -> RemoteMutationSpec { RemoteMutationSpec { - message_id: "jmap-msg-1".to_string(), + message_id: "surrogate-jmap-msg-1".to_string(), + remote_id: "jmap-msg-1".to_string(), folder_id: "mb-inbox".to_string(), - folder_checkpoint: None, + uid_validity: "jmap".to_string(), kind, } } diff --git a/crates/nuncio-store/src/db.rs b/crates/nuncio-store/src/db.rs index a40fd63..9d9d3b2 100644 --- a/crates/nuncio-store/src/db.rs +++ b/crates/nuncio-store/src/db.rs @@ -471,6 +471,8 @@ impl DatabaseEngine { id TEXT PRIMARY KEY NOT NULL, account_id TEXT NOT NULL, folder_id TEXT NOT NULL, + remote_id TEXT NOT NULL DEFAULT '', + uid_validity TEXT NOT NULL DEFAULT '', subject TEXT NOT NULL, sender TEXT NOT NULL, recipient TEXT NOT NULL, @@ -711,11 +713,67 @@ impl DatabaseEngine { self.ensure_accounts_smtp_columns().await?; self.ensure_accounts_tls_mode_columns().await?; self.ensure_accounts_dav_columns().await?; + self.ensure_messages_identity_columns().await?; self.backfill_message_fts().await?; Ok(()) } + /// Additive, backfill-safe migration that adds the `remote_id` / + /// `uid_validity` message-identity columns to a pre-existing `messages` + /// table that predated the opaque surrogate id, and installs the + /// `UNIQUE(account_id, folder_id, uid_validity, remote_id)` index that is + /// the real correctness guard against a shared protocol id (e.g. an IMAP + /// UID reused across folders/accounts) overwriting an unrelated message. + /// + /// A fresh database already gets both columns from `CREATE TABLE IF NOT + /// EXISTS messages` above, so on a fresh database the `PRAGMA table_info` + /// check finds them present and only the index is created. For a + /// pre-existing database file the columns are added with an empty-string + /// default; every legacy row is then backfilled to `remote_id = id` (the + /// old primary key, which was globally unique) so the new UNIQUE index can + /// be built without collisions. SQLite has no `ADD COLUMN IF NOT EXISTS`, + /// so column presence is checked explicitly first, making this safe to run + /// on every daemon startup. + async fn ensure_messages_identity_columns(&self) -> Result<(), DatabaseError> { + let existing_columns: Vec = sqlx::query("PRAGMA table_info(messages)") + .fetch_all(&self.pool) + .await + .map_err(DatabaseError::Query)? + .iter() + .map(|row| row.get::("name")) + .collect(); + + if !existing_columns.iter().any(|c| c == "remote_id") { + sqlx::query("ALTER TABLE messages ADD COLUMN remote_id TEXT NOT NULL DEFAULT ''") + .execute(&self.pool) + .await + .map_err(DatabaseError::Query)?; + // Seed legacy rows with a unique, non-empty remote id (the old + // primary key) so the UNIQUE identity index below is satisfiable. + sqlx::query("UPDATE messages SET remote_id = id WHERE remote_id = ''") + .execute(&self.pool) + .await + .map_err(DatabaseError::Query)?; + } + if !existing_columns.iter().any(|c| c == "uid_validity") { + sqlx::query("ALTER TABLE messages ADD COLUMN uid_validity TEXT NOT NULL DEFAULT ''") + .execute(&self.pool) + .await + .map_err(DatabaseError::Query)?; + } + + sqlx::query( + "CREATE UNIQUE INDEX IF NOT EXISTS idx_messages_identity \ + ON messages (account_id, folder_id, uid_validity, remote_id)", + ) + .execute(&self.pool) + .await + .map_err(DatabaseError::Query)?; + + Ok(()) + } + /// Additive, backfill-safe migration that adds the `imap_tls_mode` / /// `smtp_tls_mode` columns to a pre-existing `accounts` table that /// predates persisted per-protocol TLS transport modes. @@ -1081,13 +1139,15 @@ impl DatabaseEngine { sqlx::query( r#" INSERT OR REPLACE INTO messages - (id, account_id, folder_id, subject, sender, recipient, received_at, read_flag, body_plain, body_html) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + (id, account_id, folder_id, remote_id, uid_validity, subject, sender, recipient, received_at, read_flag, body_plain, body_html) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) "#, ) .bind(&email.id) .bind(&email.account_id) .bind(&email.folder_id) + .bind(&email.remote_id) + .bind(&email.uid_validity) .bind(&email.subject) .bind(&email.sender) .bind(&email.recipient) @@ -1143,9 +1203,11 @@ impl DatabaseEngine { i64, Option, Option, + String, + String, )> = sqlx::query_as( r#" - SELECT id, account_id, folder_id, subject, sender, recipient, received_at, read_flag, body_plain, body_html + SELECT id, account_id, folder_id, subject, sender, recipient, received_at, read_flag, body_plain, body_html, remote_id, uid_validity FROM messages WHERE folder_id = ? ORDER BY received_at DESC @@ -1171,6 +1233,8 @@ impl DatabaseEngine { read_flag, body_plain, body_html, + remote_id, + uid_validity, )| { let dec_plain = body_plain .map(|p| { @@ -1194,6 +1258,8 @@ impl DatabaseEngine { id, account_id, folder_id, + remote_id, + uid_validity, subject, sender, recipient, @@ -1225,9 +1291,11 @@ impl DatabaseEngine { i64, Option, Option, + String, + String, ) = sqlx::query_as( r#" - SELECT id, account_id, folder_id, subject, sender, recipient, received_at, read_flag, body_plain, body_html + SELECT id, account_id, folder_id, subject, sender, recipient, received_at, read_flag, body_plain, body_html, remote_id, uid_validity FROM messages WHERE id = ? "#, @@ -1252,6 +1320,8 @@ impl DatabaseEngine { id: row.0, account_id: row.1, folder_id: row.2, + remote_id: row.10, + uid_validity: row.11, subject: row.3, sender: row.4, recipient: row.5, @@ -1902,7 +1972,7 @@ impl DatabaseEngine { limit: usize, ) -> Result, DatabaseError> { let mut builder: sqlx::QueryBuilder<'_, sqlx::Sqlite> = sqlx::QueryBuilder::new( - "SELECT id, account_id, folder_id, subject, sender, recipient, received_at, read_flag, body_plain, body_html FROM messages " + "SELECT id, account_id, folder_id, subject, sender, recipient, received_at, read_flag, body_plain, body_html, remote_id, uid_validity FROM messages " ); if !last_id.is_empty() { builder.push("WHERE id > "); @@ -1922,6 +1992,8 @@ impl DatabaseEngine { i64, Option, Option, + String, + String, )>(); let rows = query @@ -1947,6 +2019,8 @@ impl DatabaseEngine { id: r.0, account_id: r.1, folder_id: r.2, + remote_id: r.10, + uid_validity: r.11, subject: r.3, sender: r.4, recipient: r.5, @@ -1972,7 +2046,7 @@ impl DatabaseEngine { folder_id: Option<&str>, ) -> Result, DatabaseError> { let mut builder: sqlx::QueryBuilder<'_, sqlx::Sqlite> = sqlx::QueryBuilder::new( - "SELECT id, account_id, folder_id, subject, sender, recipient, received_at, read_flag, body_plain, body_html FROM messages" + "SELECT id, account_id, folder_id, subject, sender, recipient, received_at, read_flag, body_plain, body_html, remote_id, uid_validity FROM messages" ); let mut has_filter = false; @@ -2002,6 +2076,8 @@ impl DatabaseEngine { i64, Option, Option, + String, + String, )>(); let rows = query @@ -2027,6 +2103,8 @@ impl DatabaseEngine { id: r.0, account_id: r.1, folder_id: r.2, + remote_id: r.10, + uid_validity: r.11, subject: r.3, sender: r.4, recipient: r.5, @@ -2483,6 +2561,8 @@ mod tests { id: "msg-db-100".to_string(), account_id: "acct-1".to_string(), folder_id: "INBOX".to_string(), + remote_id: "100".to_string(), + uid_validity: "1".to_string(), subject: "Database Sync Test".to_string(), sender: "alice@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), @@ -2530,6 +2610,8 @@ mod tests { id: "msg-db-corrupt".to_string(), account_id: "acct-1".to_string(), folder_id: "INBOX".to_string(), + remote_id: "corrupt".to_string(), + uid_validity: "1".to_string(), subject: "Corrupted At Rest".to_string(), sender: "alice@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), @@ -2832,6 +2914,8 @@ mod tests { id: "msg-mark-1".to_string(), account_id: "acct-1".to_string(), folder_id: "INBOX".to_string(), + remote_id: "mark-1".to_string(), + uid_validity: "1".to_string(), subject: "Mark Read Test".to_string(), sender: "alice@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), @@ -3362,6 +3446,8 @@ mod tests { id: format!("msg-{i:03}"), account_id: "acct-1".to_string(), folder_id: "inbox".to_string(), + remote_id: format!("{i}"), + uid_validity: "1".to_string(), subject: format!("Subject {i}"), sender: "alice@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), @@ -3469,11 +3555,94 @@ mod tests { ); } + /// Build a message exactly as a real sync would: an opaque surrogate id + /// hashed over its addressing coordinates, with the protocol-native id and + /// UIDVALIDITY scope carried in their own columns. + fn synced_email( + account_id: &str, + folder_id: &str, + uid_validity: &str, + remote_id: &str, + ) -> nuncio_core::model::Email { + nuncio_core::model::Email { + id: nuncio_core::model::Email::surrogate_id( + account_id, + folder_id, + uid_validity, + remote_id, + ), + account_id: account_id.to_string(), + folder_id: folder_id.to_string(), + remote_id: remote_id.to_string(), + uid_validity: uid_validity.to_string(), + subject: format!("{folder_id}/{remote_id}"), + sender: "alice@nuncio.mx".to_string(), + recipient: "bob@nuncio.mx".to_string(), + received_at: 1_700_000_000, + read: false, + body_plain: Some("body".to_string()), + body_html: None, + attachments: Vec::new(), + } + } + + /// The C1 regression: the same protocol UID reused across folders or + /// accounts must persist as DISTINCT rows (never silently overwrite), and a + /// re-sync of the SAME message must upsert the one row it already owns. + #[tokio::test] + async fn message_identity_prevents_cross_folder_and_cross_account_collision() { + let (engine, _dir) = DatabaseEngine::connect_ephemeral().await.unwrap(); + + // Same UID (5) in two folders of one account, and again in a second + // account -- the exact shapes that used to collapse to `imap-uid-5`. + let inbox = synced_email("acct-1", "INBOX", "42", "5"); + let sent = synced_email("acct-1", "Sent", "7", "5"); + let other_account = synced_email("acct-2", "INBOX", "99", "5"); + + // Every surrogate id is distinct, so none can overwrite another. + assert_ne!(inbox.id, sent.id); + assert_ne!(inbox.id, other_account.id); + assert_ne!(sent.id, other_account.id); + + engine.save_email(&inbox).await.expect("save inbox"); + engine.save_email(&sent).await.expect("save sent"); + engine + .save_email(&other_account) + .await + .expect("save other account"); + + // All three coexist as separate rows, addressed by their own folder. + assert_eq!(engine.list_messages("INBOX", 10).await.unwrap().len(), 2); + assert_eq!(engine.list_messages("Sent", 10).await.unwrap().len(), 1); + + // Re-syncing the SAME message recomputes the SAME id, so the upsert + // updates the one row rather than duplicating it. + let inbox_resynced = synced_email("acct-1", "INBOX", "42", "5"); + assert_eq!(inbox_resynced.id, inbox.id); + engine + .save_email(&inbox_resynced) + .await + .expect("re-sync upserts"); + assert_eq!( + engine.list_messages("INBOX", 10).await.unwrap().len(), + 2, + "a re-sync must upsert, not duplicate" + ); + + // The recovered row round-trips its protocol addressing columns. + let fetched = engine.get_message(&inbox.id).await.expect("get message"); + assert_eq!(fetched.remote_id, "5"); + assert_eq!(fetched.uid_validity, "42"); + assert_eq!(fetched.folder_id, "INBOX"); + } + fn export_test_email(id: &str, account_id: &str, folder_id: &str) -> nuncio_core::model::Email { nuncio_core::model::Email { id: id.to_string(), account_id: account_id.to_string(), folder_id: folder_id.to_string(), + remote_id: id.to_string(), + uid_validity: "1".to_string(), subject: format!("Subject {id}"), sender: "alice@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), diff --git a/crates/nuncio-store/src/search.rs b/crates/nuncio-store/src/search.rs index ed6d1ad..0befdf5 100644 --- a/crates/nuncio-store/src/search.rs +++ b/crates/nuncio-store/src/search.rs @@ -105,6 +105,8 @@ mod tests { id: id.to_string(), account_id: "acct-1".to_string(), folder_id: "inbox".to_string(), + remote_id: id.to_string(), + uid_validity: "1".to_string(), subject: subject.to_string(), sender: "alice@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), diff --git a/crates/nunciod/src/grpc.rs b/crates/nunciod/src/grpc.rs index 23f0f66..bfa15e1 100644 --- a/crates/nunciod/src/grpc.rs +++ b/crates/nunciod/src/grpc.rs @@ -1592,6 +1592,9 @@ fn synthetic_preview_email(id: &str) -> nuncio_core::model::Email { id: id.to_string(), account_id: "acct-1".to_string(), folder_id: "inbox".to_string(), + // Synthetic preview only -- never persisted or mutated remotely. + remote_id: id.to_string(), + uid_validity: String::new(), subject: "Test Subject".to_string(), sender: "test@nuncio.mx".to_string(), recipient: "me@nuncio.mx".to_string(), @@ -3434,6 +3437,8 @@ mod tests { id: id.to_string(), account_id: "acct-mail-1".to_string(), folder_id: folder_id.to_string(), + remote_id: id.to_string(), + uid_validity: "1".to_string(), subject: subject.to_string(), sender: "alice@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), @@ -4320,6 +4325,8 @@ mod tests { id: "msg-filters-1".to_string(), account_id: "acct-1".to_string(), folder_id: "inbox".to_string(), + remote_id: "1".to_string(), + uid_validity: "1".to_string(), subject: subject.to_string(), sender: "alice@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), @@ -4420,6 +4427,8 @@ mod tests { id: "msg-1".to_string(), account_id: "acct-1".to_string(), folder_id: "inbox".to_string(), + remote_id: "1".to_string(), + uid_validity: "1".to_string(), subject: "anything".to_string(), sender: "a@b.com".to_string(), recipient: "c@d.com".to_string(), @@ -4562,6 +4571,8 @@ mod tests { id: "msg-preview-1".to_string(), account_id: "acct-1".to_string(), folder_id: "inbox".to_string(), + remote_id: "preview-1".to_string(), + uid_validity: "1".to_string(), subject: "Urgent Board Meeting".to_string(), sender: "alice@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), @@ -4728,6 +4739,8 @@ mod tests { id: "msg-1".to_string(), account_id: "acct-1".to_string(), folder_id: "inbox".to_string(), + remote_id: "1".to_string(), + uid_validity: "1".to_string(), subject: "Urgent Meeting".to_string(), sender: "a@b.com".to_string(), recipient: "c@d.com".to_string(), @@ -4882,6 +4895,8 @@ mod tests { id: "msg-1".to_string(), account_id: "acct-1".to_string(), folder_id: "inbox".to_string(), + remote_id: "1".to_string(), + uid_validity: "1".to_string(), subject: "Urgent Meeting".to_string(), sender: "a@b.com".to_string(), recipient: "c@d.com".to_string(), diff --git a/crates/nunciod/src/outbox.rs b/crates/nunciod/src/outbox.rs index f022ea5..062c38a 100644 --- a/crates/nunciod/src/outbox.rs +++ b/crates/nunciod/src/outbox.rs @@ -204,7 +204,7 @@ async fn execute_one( match mutation.mutation_type.as_str() { "MOVE" | "COPY" | "FLAG" | "UNFLAG" | "DELETE" => { - dispatch_mailbox_mutation(db, env, mutation, &email, &payload).await + dispatch_mailbox_mutation(env, mutation, &email, &payload).await } "FORWARD" => dispatch_forward(db, env, &email, &payload).await, "WEBHOOK" => dispatch_webhook(env, mutation, &email, &payload).await, @@ -213,10 +213,10 @@ async fn execute_one( } /// Execute a mailbox-affecting mutation (move/copy/flag/unflag/delete) against -/// the account's mail backend, threading the source folder's stored checkpoint -/// so the backend can enforce its UIDVALIDITY guard. +/// the account's mail backend, recovering the remote addressing (protocol id + +/// UIDVALIDITY scope) from the stored message row so the backend can enforce +/// its UIDVALIDITY guard. async fn dispatch_mailbox_mutation( - db: &DatabaseEngine, env: &dyn RemoteExecutionEnv, mutation: &PendingRemoteMutation, email: &nuncio_core::model::Email, @@ -245,23 +245,20 @@ async fn dispatch_mailbox_mutation( other => return Disposition::Permanent(format!("unexpected mailbox mutation '{other}'")), }; - let folder_checkpoint = match db - .get_folder_sync_state(&email.account_id, &email.folder_id) - .await - { - Ok(checkpoint) => checkpoint, - Err(e) => return Disposition::Retry(format!("failed to read folder checkpoint: {e}")), - }; - let backend = match env.mail_backend(&email.account_id).await { Ok(backend) => backend, Err(e) => return Disposition::Retry(format!("failed to build mail backend: {e}")), }; + // Recover the remote addressing from the stored message row -- the + // protocol-native id and the UIDVALIDITY scope it was captured under -- + // rather than parsing it out of the opaque surrogate id. The backend still + // SELECT-verifies the UIDVALIDITY before mutating. let spec = RemoteMutationSpec { message_id: email.id.clone(), + remote_id: email.remote_id.clone(), folder_id: email.folder_id.clone(), - folder_checkpoint, + uid_validity: email.uid_validity.clone(), kind, }; diff --git a/crates/nunciod/src/sync.rs b/crates/nunciod/src/sync.rs index bbb395c..bbab07d 100644 --- a/crates/nunciod/src/sync.rs +++ b/crates/nunciod/src/sync.rs @@ -628,6 +628,8 @@ mod tests { id: id.to_string(), account_id: "acct-mock-1".to_string(), folder_id: folder_id.to_string(), + remote_id: id.to_string(), + uid_validity: "1".to_string(), subject: subject.to_string(), sender: "alice@nuncio.mx".to_string(), recipient: "bob@nuncio.mx".to_string(), @@ -864,11 +866,16 @@ mod tests { // The wiremock stubs advertise 1 folder containing 1 message. assert_eq!(synced, 1); + // The message persists under an opaque surrogate id; it is recovered by + // folder, and its protocol-native JMAP object id round-trips in + // `remote_id`. let persisted = db - .get_message("jmap-msg-1") + .list_messages("inbox", 10) .await .expect("jmap message persisted"); - assert_eq!(persisted.subject, "Welcome to JMAP Sync"); + assert_eq!(persisted.len(), 1); + assert_eq!(persisted[0].subject, "Welcome to JMAP Sync"); + assert_eq!(persisted[0].remote_id, "jmap-msg-1"); assert_eq!( events.recv().await.expect("start event"), diff --git a/crates/nunciod/tests/filters_triage_e2e_test.rs b/crates/nunciod/tests/filters_triage_e2e_test.rs index bb34869..60689d3 100644 --- a/crates/nunciod/tests/filters_triage_e2e_test.rs +++ b/crates/nunciod/tests/filters_triage_e2e_test.rs @@ -43,6 +43,8 @@ fn sample_email(id: &str, subject: &str) -> Email { id: id.to_string(), account_id: ACCOUNT_ID.to_string(), folder_id: "inbox".to_string(), + remote_id: id.to_string(), + uid_validity: "1".to_string(), subject: subject.to_string(), sender: "alice@nuncio.mx".to_string(), recipient: "triage-e2e@nuncio.mx".to_string(), diff --git a/crates/nunciod/tests/outbox_executor_e2e_test.rs b/crates/nunciod/tests/outbox_executor_e2e_test.rs index 6f823ff..d88509a 100644 --- a/crates/nunciod/tests/outbox_executor_e2e_test.rs +++ b/crates/nunciod/tests/outbox_executor_e2e_test.rs @@ -103,9 +103,11 @@ fn sample_account() -> nuncio_core::AccountConfig { fn sample_email(subject: &str) -> Email { Email { - id: "imap-uid-42".to_string(), + id: "surrogate-inbox-42".to_string(), account_id: ACCOUNT_ID.to_string(), folder_id: FOLDER_ID.to_string(), + remote_id: "42".to_string(), + uid_validity: "1".to_string(), subject: subject.to_string(), sender: "alice@nuncio.mx".to_string(), recipient: "owner@nuncio.mx".to_string(), @@ -161,11 +163,13 @@ async fn move_rule_executes_against_backend_and_completes_with_uidvalidity_check let applied = env.backend.applied_mutations(); assert_eq!(applied.len(), 1, "the backend op must genuinely run"); - assert_eq!(applied[0].message_id, "imap-uid-42"); + // Addressing is recovered from the stored row's columns, not by parsing the + // opaque surrogate id: the protocol-native UID and the UIDVALIDITY scope it + // was captured under, so the backend can enforce its guard. + assert_eq!(applied[0].message_id, "surrogate-inbox-42"); + assert_eq!(applied[0].remote_id, "42"); assert_eq!(applied[0].folder_id, FOLDER_ID); - // The source folder's stored checkpoint (carrying UIDVALIDITY) is threaded - // through so the backend can enforce its guard. - assert_eq!(applied[0].folder_checkpoint.as_deref(), Some(CHECKPOINT)); + assert_eq!(applied[0].uid_validity, "1"); assert_eq!( applied[0].kind, RemoteMutationKind::Move { diff --git a/crates/nunciod/tests/spine_e2e_test.rs b/crates/nunciod/tests/spine_e2e_test.rs index 058225e..187ef0d 100644 --- a/crates/nunciod/tests/spine_e2e_test.rs +++ b/crates/nunciod/tests/spine_e2e_test.rs @@ -77,6 +77,8 @@ fn mock_inbound_email(id: &str, subject: &str, body: &str) -> Email { id: id.to_string(), account_id: ACCOUNT_ID.to_string(), folder_id: "inbox".to_string(), + remote_id: id.to_string(), + uid_validity: "1".to_string(), subject: subject.to_string(), sender: "alice@nuncio.mx".to_string(), recipient: ACCOUNT_EMAIL.to_string(), -- 2.54.0 From fdac4d29e233284bd9d7ecb5eace09de9f322bc6 Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 31 Jul 2026 13:55:31 -0500 Subject: [PATCH 4/4] fix(store): make message-identity migration atomic and self-healing The identity migration ran ADD COLUMN, backfill, and CREATE UNIQUE INDEX as separate autocommit statements with the backfill gated on having just added the column. A crash between the ADD and the backfill could leave legacy rows at remote_id='' with the column present, so the next startup skipped the backfill and the index creation then collided on (account, folder, uid_validity='', remote_id='') -- wedging the daemon. Run the whole step in one transaction (a crash rolls back atomically) and make the backfill unconditional (idempotent UPDATE ... WHERE remote_id = '') so any committed partial state is completed before the index is built. No-op on a fresh or already-migrated database. Refs #281 --- crates/nuncio-store/src/db.rs | 108 +++++++++++++++++++++++++++++----- 1 file changed, 92 insertions(+), 16 deletions(-) diff --git a/crates/nuncio-store/src/db.rs b/crates/nuncio-store/src/db.rs index 9d9d3b2..5dd7ae9 100644 --- a/crates/nuncio-store/src/db.rs +++ b/crates/nuncio-store/src/db.rs @@ -728,13 +728,22 @@ impl DatabaseEngine { /// /// A fresh database already gets both columns from `CREATE TABLE IF NOT /// EXISTS messages` above, so on a fresh database the `PRAGMA table_info` - /// check finds them present and only the index is created. For a - /// pre-existing database file the columns are added with an empty-string - /// default; every legacy row is then backfilled to `remote_id = id` (the - /// old primary key, which was globally unique) so the new UNIQUE index can - /// be built without collisions. SQLite has no `ADD COLUMN IF NOT EXISTS`, - /// so column presence is checked explicitly first, making this safe to run - /// on every daemon startup. + /// check finds them present and the backfill matches no rows -- only the + /// index is created. For a pre-existing database file the columns are added + /// with an empty-string default and every legacy row is backfilled to + /// `remote_id = id` (the old primary key, which was globally unique) so the + /// UNIQUE index can be built without collisions. SQLite has no `ADD COLUMN + /// IF NOT EXISTS`, so column presence is checked explicitly first, making + /// this safe to run on every daemon startup. + /// + /// The whole step runs in a single transaction, and the backfill is + /// unconditional (idempotent `WHERE remote_id = ''`) rather than gated on + /// having just added the column. Both properties make a partial run + /// self-healing: a crash mid-migration rolls back atomically, and even a + /// committed intermediate state (column present but a legacy row still + /// carrying the empty default) is completed by the next startup's backfill + /// before the index is built -- so the daemon can never wedge on an index + /// collision it could have avoided. async fn ensure_messages_identity_columns(&self) -> Result<(), DatabaseError> { let existing_columns: Vec = sqlx::query("PRAGMA table_info(messages)") .fetch_all(&self.pool) @@ -744,33 +753,41 @@ impl DatabaseEngine { .map(|row| row.get::("name")) .collect(); + let mut tx = self.pool.begin().await.map_err(DatabaseError::Query)?; + if !existing_columns.iter().any(|c| c == "remote_id") { sqlx::query("ALTER TABLE messages ADD COLUMN remote_id TEXT NOT NULL DEFAULT ''") - .execute(&self.pool) - .await - .map_err(DatabaseError::Query)?; - // Seed legacy rows with a unique, non-empty remote id (the old - // primary key) so the UNIQUE identity index below is satisfiable. - sqlx::query("UPDATE messages SET remote_id = id WHERE remote_id = ''") - .execute(&self.pool) + .execute(&mut *tx) .await .map_err(DatabaseError::Query)?; } if !existing_columns.iter().any(|c| c == "uid_validity") { sqlx::query("ALTER TABLE messages ADD COLUMN uid_validity TEXT NOT NULL DEFAULT ''") - .execute(&self.pool) + .execute(&mut *tx) .await .map_err(DatabaseError::Query)?; } + // Seed any legacy row still carrying the empty-string default with a + // unique, non-empty remote id (the old primary key) so the UNIQUE + // identity index is satisfiable. Unconditional so a partial prior run + // is always completed before the index is (re)built; a no-op on a fresh + // or already-migrated database. + sqlx::query("UPDATE messages SET remote_id = id WHERE remote_id = ''") + .execute(&mut *tx) + .await + .map_err(DatabaseError::Query)?; + sqlx::query( "CREATE UNIQUE INDEX IF NOT EXISTS idx_messages_identity \ ON messages (account_id, folder_id, uid_validity, remote_id)", ) - .execute(&self.pool) + .execute(&mut *tx) .await .map_err(DatabaseError::Query)?; + tx.commit().await.map_err(DatabaseError::Query)?; + Ok(()) } @@ -3636,6 +3653,65 @@ mod tests { assert_eq!(fetched.folder_id, "INBOX"); } + /// A crash between `ADD COLUMN remote_id` and its backfill can commit an + /// intermediate state -- the column present but a legacy row still carrying + /// the empty-string default -- with the UNIQUE index not yet built. The + /// migration must complete that backfill on the next open and build the + /// index without a collision, rather than wedging the daemon. + #[tokio::test] + async fn messages_identity_migration_completes_from_a_partial_run() { + let (engine, _dir) = DatabaseEngine::connect_ephemeral().await.unwrap(); + + // Reproduce the partial state: drop the identity index, then insert two + // rows in the same folder that both still hold the empty default. With + // the index gone this is permitted, and the two would collide on + // (account, folder, uid_validity='', remote_id='') if it were rebuilt now. + sqlx::query("DROP INDEX IF EXISTS idx_messages_identity") + .execute(engine.pool()) + .await + .unwrap(); + for id in ["legacy-a", "legacy-b"] { + sqlx::query( + "INSERT INTO messages \ + (id, account_id, folder_id, remote_id, uid_validity, subject, sender, recipient, received_at, read_flag) \ + VALUES (?, 'acct-1', 'INBOX', '', '', 'subj', 'a@nuncio.mx', 'b@nuncio.mx', 0, 0)", + ) + .bind(id) + .execute(engine.pool()) + .await + .unwrap(); + } + + // Re-running the migration must finish the backfill and build the index. + engine + .ensure_messages_identity_columns() + .await + .expect("a partial migration must complete on the next open"); + + // Both legacy rows survive with a unique, non-empty backfilled remote id. + assert_eq!( + engine.get_message("legacy-a").await.unwrap().remote_id, + "legacy-a" + ); + assert_eq!( + engine.get_message("legacy-b").await.unwrap().remote_id, + "legacy-b" + ); + + // The unique identity index is now present, and re-running is idempotent. + let index: Option<(String,)> = sqlx::query_as( + "SELECT name FROM sqlite_master WHERE type = 'index' AND name = 'idx_messages_identity'", + ) + .fetch_optional(engine.pool()) + .await + .unwrap(); + assert!(index.is_some(), "the unique identity index must be built"); + engine + .ensure_messages_identity_columns() + .await + .expect("re-running an already-complete migration is a no-op"); + } + fn export_test_email(id: &str, account_id: &str, folder_id: &str) -> nuncio_core::model::Email { nuncio_core::model::Email { id: id.to_string(), -- 2.54.0