From 43b95e76142c62a7df7cb786a385f57d9c431df4 Mon Sep 17 00:00:00 2001 From: James Maes Date: Wed, 29 Jul 2026 20:12:17 -0500 Subject: [PATCH 1/5] fix(accounts): roll back orphaned keyring secret on add_account failure (#229) If set_secret succeeds but the subsequent save_account fails, the credential was left in the keyring with no account row referencing it. add_account now deletes the just-written secret before returning, appending any rollback-delete failure to the original error message without masking it. Refs #197, #169 Co-authored-by: James Maes --- crates/nunciod/src/grpc.rs | 125 +++++++++++++++++++++++++++++++++++-- 1 file changed, 121 insertions(+), 4 deletions(-) diff --git a/crates/nunciod/src/grpc.rs b/crates/nunciod/src/grpc.rs index bab507a..0a5a308 100644 --- a/crates/nunciod/src/grpc.rs +++ b/crates/nunciod/src/grpc.rs @@ -340,14 +340,33 @@ impl Accounts for AccountsGrpcService { // SQLite. If this fails, the account row below is deliberately // never saved, so a failed credential write can never leave behind // an account config with no retrievable password. + // + // If the credential write succeeds but the account row then fails to + // persist, the two steps are no longer in sync: without a rollback, + // the keyring would keep an orphaned secret that no account row + // references and that `ListAccounts` can never surface again. So on + // that failure we delete the just-written secret before returning, + // restoring the "no credential without a matching account" invariant. + // The original persistence error is always what the caller sees -- + // a rollback failure is reported alongside it, never in place of it. self.secrets .set_secret(&config.keyring_secret_key, &req.password) .map_err(|e| Status::internal(format!("failed to store credential in vault: {e}")))?; - self.db - .save_account(&config) - .await - .map_err(|e| Status::internal(format!("failed to persist account: {e}")))?; + if let Err(e) = self.db.save_account(&config).await { + let mut message = format!("failed to persist account: {e}"); + if let Err(rollback_err) = self.secrets.delete_secret(&config.keyring_secret_key) { + tracing::warn!( + keyring_secret_key = %config.keyring_secret_key, + error = %rollback_err, + "failed to roll back orphaned keyring secret after add_account persistence failure" + ); + message.push_str(&format!( + " (and failed to roll back the credential: {rollback_err})" + )); + } + return Err(Status::internal(message)); + } Ok(Response::new(AddAccountResponse { id: config.id })) } @@ -2654,6 +2673,104 @@ mod tests { ); } + /// Proves that when `set_secret` succeeds but the subsequent + /// `save_account` genuinely fails, `add_account` rolls back the + /// just-written credential instead of leaving it orphaned in the + /// keyring. The failure is forced deterministically by closing the + /// `DatabaseEngine`'s connection pool before the RPC is made, so + /// `save_account` fails for real (not a simulated/fabricated error). + #[tokio::test] + async fn add_account_rolls_back_keyring_secret_when_save_account_fails() { + let (db, _dir) = DatabaseEngine::connect_ephemeral() + .await + .expect("connect ephemeral test db"); + let db = Arc::new(db); + let secrets = Arc::new(SecretManager::mock()); + let (addr, _handle) = spawn_test_server_with( + Arc::new(EventBus::new()), + db.clone(), + Arc::new(FilterEngine::new(Vec::new()).expect("empty rule set")), + secrets.clone(), + "correct-token", + ) + .await; + + // Close the pool out from under the still-running server so the + // upcoming `save_account` call fails with a real "pool closed" + // error rather than anything contrived. + db.close().await; + + let mut client = AccountsClient::connect(format!("http://{addr}")) + .await + .expect("client connects"); + let err = client + .add_account(authed_bearer_request(AddAccountRequest { + config: Some(sample_account_config_proto( + "acct-rollback-1", + "nuncio/acct-rollback-1", + )), + password: "will-be-orphaned-without-rollback".to_string(), + })) + .await + .expect_err("save_account must fail once the pool is closed"); + assert_eq!(err.code(), Code::Internal); + + // The rollback must have deleted the secret written just before the + // failed persistence attempt -- not left it behind, unreferenced by + // any account row. + let after_rollback = secrets.get_secret("nuncio/acct-rollback-1"); + assert!( + after_rollback.is_err(), + "keyring secret should have been rolled back, but was still readable: {after_rollback:?}" + ); + } + + /// Proves the error surfaced to the caller after a rollback describes + /// the original persistence failure, never the rollback itself -- the + /// rollback is a failure-path side effect, not a replacement error. + #[tokio::test] + async fn add_account_rollback_preserves_the_original_persistence_error() { + let (db, _dir) = DatabaseEngine::connect_ephemeral() + .await + .expect("connect ephemeral test db"); + let db = Arc::new(db); + let secrets = Arc::new(SecretManager::mock()); + let (addr, _handle) = spawn_test_server_with( + Arc::new(EventBus::new()), + db.clone(), + Arc::new(FilterEngine::new(Vec::new()).expect("empty rule set")), + secrets.clone(), + "correct-token", + ) + .await; + + db.close().await; + + let mut client = AccountsClient::connect(format!("http://{addr}")) + .await + .expect("client connects"); + let err = client + .add_account(authed_bearer_request(AddAccountRequest { + config: Some(sample_account_config_proto( + "acct-rollback-2", + "nuncio/acct-rollback-2", + )), + password: "irrelevant-password".to_string(), + })) + .await + .expect_err("save_account must fail once the pool is closed"); + + let message = err.message(); + assert!( + message.contains("failed to persist account"), + "error message must describe the persistence failure, got: {message}" + ); + assert!( + !message.contains("failed to store credential in vault"), + "error message must not be the unrelated set_secret failure message, got: {message}" + ); + } + // ---- Mail ---- use nuncio_proto::v1::mail_client::MailClient; -- 2.54.0 From 0b8307a268762a50381d421a97ce38914451cb1c Mon Sep 17 00:00:00 2001 From: James Maes Date: Wed, 29 Jul 2026 20:57:55 -0500 Subject: [PATCH 2/5] M5: Account lifecycle over gRPC (edit/delete/test) + persist TLS mode (#230) * feat(store): persist per-account IMAP/SMTP TLS modes; add account CRUD Adds a backfill-safe imap_tls_mode/smtp_tls_mode column migration, binds+reads both modes in save_account/list_accounts (previously validated then silently discarded and hardcoded back to ImplicitTls), and adds get_account/delete_account. Regression test proves the modes survive a save/list/get round-trip. Refs #194 #182 * feat(mail): branch IMAP/SMTP transport on TlsMode; add connection probes Threads TlsMode through ImapEngine/ImapDualSocketManager and SmtpTransportEngine so implicit TLS, STARTTLS, and plain are genuine transport paths (a unified MailStream plus a real IMAP STARTTLS negotiation), not validate-and-discard. Adds bounded connect timeout and per-engine probe_connection backing TestAccountConnection, with mock/loopback-only setup tests for each mode. Refs #194 #182 * feat(accounts): edit/delete/test account lifecycle over gRPC Implements Accounts UpdateAccount/RemoveAccount/TestAccountConnection in the daemon (update mirrors add_account's keyring rollback discipline; test probes real IMAP+SMTP via an injectable tester, honoring persisted TLS modes), wires the configured TLS modes into build_mail_backend/ build_smtp_sender, and replaces the fabricated CLI account show/edit/delete/test handlers with real thin gRPC clients. Adds an offline full-daemon lifecycle E2E and unauth-rejection coverage for all three new RPCs. Refs #194 #182 * fix(accounts): restore prior credential on update rollback A password rotation that fails to persist previously deleted the just-written secret, destroying the working credential the key already held and orphaning the still-present account row. Snapshot the prior secret first and restore it on save failure (falling back to delete only when none existed). Regression tests force save_account to fail and assert the original secret survives. Refs #194 #182 * docs(accounts): scope TestAccountConnection SMTP claim to reachability The SMTP leg only dials and negotiates TLS/STARTTLS; it performs no AUTH (lettre exposes no clean auth-only probe on AsyncSmtpTransport), while IMAP genuinely logs in. Clarify the proto doc for smtp_ok/smtp_error and the CLI output wording so nothing implies SMTP authentication was verified. Doc-only proto change; golden unchanged. Refs #194 #182 --------- Co-authored-by: James Maes --- crates/nuncio-cli/src/args.rs | 6 + crates/nuncio-cli/src/runner.rs | 446 ++++++++++++++---- crates/nuncio-mail/src/imap.rs | 338 ++++++++++++- crates/nuncio-mail/src/smtp.rs | 145 ++++-- .../proto/nuncio/v1/descriptor.bin | Bin 12014 -> 12734 bytes .../nuncio-proto/proto/nuncio/v1/nuncio.proto | 75 +++ crates/nuncio-store/src/db.rs | 203 +++++++- crates/nunciod/src/grpc.rs | 438 ++++++++++++++++- crates/nunciod/src/send.rs | 1 + crates/nunciod/src/sync.rs | 1 + .../tests/accounts_lifecycle_e2e_test.rs | 244 ++++++++++ crates/nunciod/tests/calendar_e2e_test.rs | 5 +- crates/nunciod/tests/contacts_e2e_test.rs | 5 +- crates/nunciod/tests/grpc_auth_test.rs | 48 ++ crates/nunciod/tests/spine_e2e_test.rs | 5 +- 15 files changed, 1814 insertions(+), 146 deletions(-) create mode 100644 crates/nunciod/tests/accounts_lifecycle_e2e_test.rs diff --git a/crates/nuncio-cli/src/args.rs b/crates/nuncio-cli/src/args.rs index abd2f38..96bba71 100644 --- a/crates/nuncio-cli/src/args.rs +++ b/crates/nuncio-cli/src/args.rs @@ -258,6 +258,12 @@ pub enum AccountSubcommand { /// Updated SMTP server port. #[arg(long, help = "Updated SMTP server port")] smtp_port: Option, + /// Updated IMAP transport mode (implicit_tls, start_tls, plain). + #[arg(long, help = "Updated IMAP transport mode")] + imap_mode: Option, + /// Updated SMTP transport mode (implicit_tls, start_tls, plain). + #[arg(long, help = "Updated SMTP transport mode")] + smtp_mode: Option, }, /// Remove a configured account profile. Delete { diff --git a/crates/nuncio-cli/src/runner.rs b/crates/nuncio-cli/src/runner.rs index e5a3a53..68c34f3 100644 --- a/crates/nuncio-cli/src/runner.rs +++ b/crates/nuncio-cli/src/runner.rs @@ -291,45 +291,32 @@ impl HeadlessRunner { ) .await } - AccountSubcommand::Show { id } => { - let accounts = self.db.list_accounts().await.unwrap_or_default(); - let acct = accounts.into_iter().find(|a| a.id == *id); - if json_mode { - format_json(&json!({ "account": acct })) - } else if let Some(a) = acct { - format!("Account {}: Email: {}", a.id, a.email_address) - } else { - format!("Account '{}' not found", id) - } - } + AccountSubcommand::Show { id } => self.handle_account_show(id, json_mode).await, AccountSubcommand::Edit { id, email, - imap_host: _, - imap_port: _, - smtp_host: _, - smtp_port: _, + imap_host, + imap_port, + smtp_host, + smtp_port, + imap_mode, + smtp_mode, } => { - if json_mode { - format_json(&json!({ "status": "updated", "id": id, "email": email })) - } else { - format!("Account '{}' updated successfully.", id) - } - } - AccountSubcommand::Delete { id } => { - if json_mode { - format_json(&json!({ "status": "deleted", "id": id })) - } else { - format!("Account '{}' removed.", id) - } - } - AccountSubcommand::Test { id } => { - if json_mode { - format_json(&json!({ "status": "ok", "id": id, "latency_ms": 24 })) - } else { - format!("✓ Account '{}' connection test OK (24ms latency).", id) - } + self.handle_account_edit( + id, + email.as_deref(), + imap_host.as_deref(), + *imap_port, + smtp_host.as_deref(), + *smtp_port, + imap_mode.as_deref(), + smtp_mode.as_deref(), + json_mode, + ) + .await } + AccountSubcommand::Delete { id } => self.handle_account_delete(id, json_mode).await, + AccountSubcommand::Test { id } => self.handle_account_test(id, json_mode).await, }, Commands::Mail { action } => match action { MailSubcommand::Sync => self.handle_sync(json_mode).await, @@ -1753,6 +1740,229 @@ impl HeadlessRunner { } } + /// Fetches every account from the daemon and returns the one matching + /// `id`, or an error string. There is deliberately no single-account + /// `GetAccount` RPC in the contract, so `account show`/`edit` filter the + /// `ListAccounts` result client-side -- always over the real daemon API, + /// never this runner's own ephemeral local `db`. + async fn fetch_account_by_id( + &self, + id: &str, + ) -> Result { + let mut client = self.connect_accounts_client().await?; + let accounts = client + .list_accounts(nuncio_proto::v1::ListAccountsRequest {}) + .await + .map_err(|status| format!("nunciod daemon rejected list_accounts: {status}"))? + .into_inner() + .accounts; + accounts + .into_iter() + .find(|a| a.id == id) + .ok_or_else(|| format!("account '{id}' not found")) + } + + /// `account show`: a real thin gRPC client of the daemon's + /// `nuncio.v1.Accounts/ListAccounts` API, filtered client-side to a single + /// account. Reads ONLY the daemon's persistent store -- never this + /// runner's own ephemeral local `db`. + async fn handle_account_show(&self, id: &str, json_mode: bool) -> String { + let account = match self.fetch_account_by_id(id).await { + Ok(account) => account, + Err(e) => return Self::render_error(&e, json_mode), + }; + + if json_mode { + format_json(&json!({ + "id": account.id, + "name": account.name, + "email_address": account.email_address, + "protocol": account.protocol().as_str_name(), + "server_host": account.server_host, + "server_port": account.server_port, + "smtp_host": account.smtp_host, + "smtp_port": account.smtp_port, + "use_tls": account.use_tls, + "imap_tls_mode": account.imap_tls_mode().as_str_name(), + "smtp_tls_mode": account.smtp_tls_mode().as_str_name(), + "keyring_secret_key": account.keyring_secret_key, + "sync_interval_secs": account.sync_interval_secs, + })) + } else { + format!( + "Account [{}] {} <{}> IMAP {}:{} ({}) SMTP {}:{} ({})", + account.id, + account.name, + account.email_address, + account.server_host, + account.server_port, + account.imap_tls_mode().as_str_name(), + account.smtp_host, + account.smtp_port, + account.smtp_tls_mode().as_str_name(), + ) + } + } + + /// `account edit`: a real thin gRPC client of the daemon's + /// `nuncio.v1.Accounts/UpdateAccount` API. Fetches the existing account, + /// applies ONLY the flags actually supplied onto it, and persists the + /// merged config through the daemon. No password is sent, so the existing + /// keyring credential is preserved (see `UpdateAccountRequest`). + #[allow(clippy::too_many_arguments)] + async fn handle_account_edit( + &self, + id: &str, + email: Option<&str>, + imap_host: Option<&str>, + imap_port: Option, + smtp_host: Option<&str>, + smtp_port: Option, + imap_mode: Option<&str>, + smtp_mode: Option<&str>, + json_mode: bool, + ) -> String { + let mut config = match self.fetch_account_by_id(id).await { + Ok(config) => config, + Err(e) => return Self::render_error(&e, json_mode), + }; + + if let Some(email) = email { + config.email_address = email.to_string(); + } + if let Some(host) = imap_host { + config.server_host = host.to_string(); + } + if let Some(port) = imap_port { + config.server_port = u32::from(port); + } + if let Some(host) = smtp_host { + config.smtp_host = host.to_string(); + } + if let Some(port) = smtp_port { + config.smtp_port = u32::from(port); + } + if let Some(mode) = imap_mode { + match parse_tls_mode(mode) { + Ok(mode) => config.imap_tls_mode = map_tls_mode_to_proto(mode).into(), + Err(e) => return Self::render_error(&e, json_mode), + } + } + if let Some(mode) = smtp_mode { + match parse_tls_mode(mode) { + Ok(mode) => config.smtp_tls_mode = map_tls_mode_to_proto(mode).into(), + Err(e) => return Self::render_error(&e, json_mode), + } + } + + let mut client = match self.connect_accounts_client().await { + Ok(client) => client, + Err(e) => return Self::render_error(&e, json_mode), + }; + + match client + .update_account(nuncio_proto::v1::UpdateAccountRequest { + config: Some(config), + password: None, + }) + .await + { + Ok(_) => { + if json_mode { + format_json(&json!({ "status": "updated", "id": id })) + } else { + format!("Account '{id}' updated via nunciod daemon.") + } + } + Err(status) => Self::render_error( + &format!("nunciod daemon rejected update_account: {status}"), + json_mode, + ), + } + } + + /// `account delete`: a real thin gRPC client of the daemon's + /// `nuncio.v1.Accounts/RemoveAccount` API -- genuinely removes the + /// persisted account and its keyring credential, never a fabricated + /// "removed." string. + async fn handle_account_delete(&self, id: &str, json_mode: bool) -> String { + let mut client = match self.connect_accounts_client().await { + Ok(client) => client, + Err(e) => return Self::render_error(&e, json_mode), + }; + + match client + .remove_account(nuncio_proto::v1::RemoveAccountRequest { id: id.to_string() }) + .await + { + Ok(_) => { + if json_mode { + format_json(&json!({ "status": "deleted", "id": id })) + } else { + format!("Account '{id}' removed via nunciod daemon.") + } + } + Err(status) => Self::render_error( + &format!("nunciod daemon rejected remove_account: {status}"), + json_mode, + ), + } + } + + /// `account test`: a real thin gRPC client of the daemon's + /// `nuncio.v1.Accounts/TestAccountConnection` API. Reports the GENUINE + /// per-protocol outcome the daemon measured -- never a fabricated + /// "connection test OK (24ms latency)". The two legs verify different + /// depths (see the RPC's proto doc): IMAP is a full login, SMTP is + /// transport reachability only (no AUTH), so the output labels them + /// distinctly and never implies SMTP authentication was verified. + async fn handle_account_test(&self, id: &str, json_mode: bool) -> String { + let mut client = match self.connect_accounts_client().await { + Ok(client) => client, + Err(e) => return Self::render_error(&e, json_mode), + }; + + match client + .test_account_connection(nuncio_proto::v1::TestAccountConnectionRequest { + id: id.to_string(), + }) + .await + { + Ok(response) => { + let report = response.into_inner(); + if json_mode { + format_json(&json!({ + "id": id, + "imap_ok": report.imap_ok, + "smtp_ok": report.smtp_ok, + "imap_error": report.imap_error, + "smtp_error": report.smtp_error, + })) + } else { + // Label each leg by what it actually verifies: IMAP is a + // full login, SMTP is transport reachability only (no + // AUTH). The wording must never imply SMTP authentication + // was checked when it was not. + let imap = match &report.imap_error { + Some(e) => format!("IMAP (login): FAIL ({e})"), + None if report.imap_ok => "IMAP (login): OK".to_string(), + None => "IMAP (login): FAIL".to_string(), + }; + let smtp = match &report.smtp_error { + Some(e) => format!("SMTP (reachability, no auth): FAIL ({e})"), + None if report.smtp_ok => "SMTP (reachability, no auth): OK".to_string(), + None => "SMTP (reachability, no auth): FAIL".to_string(), + }; + format!("Connection test for '{id}'\n {imap}\n {smtp}") + } + } + Err(status) => Self::render_error( + &format!("nunciod daemon rejected test_account_connection: {status}"), + json_mode, + ), + } + } + /// Resolves the gRPC bearer token from the injected vault and dials the /// `nuncio.v1.Accounts` service at `self.grpc_addr`, shared by /// [`Self::handle_add_account`] and [`Self::handle_accounts_list`]. @@ -2246,60 +2456,6 @@ mod tests { assert!(bad_smtp_mode.starts_with("Error: invalid tls mode")); } - #[tokio::test] - async fn headless_runner_executes_all_pure_noun_verb_commands() { - let runner = HeadlessRunner::ephemeral() - .await - .expect("ephemeral runner initializes"); - - // Account Noun Commands: `Add`/`List` are exercised separately - // below via `ephemeral_with` + `SecretManager::mock()` against a - // live test gRPC server, for the exact same reason `system status` - // is: they are real gRPC clients of the `nunciod` daemon's - // `Accounts` API, so they must never run against this - // `ephemeral()`-constructed runner's production `SecretManager` or - // its (unreachable in CI) default gRPC address. `Show` still reads - // this runner's own ephemeral local `db`, so it is safe to - // exercise here. - let acct_show = runner - .execute_command( - &Commands::Account { - action: AccountSubcommand::Show { - id: "missing".to_string(), - }, - }, - false, - ) - .await; - assert!(acct_show.contains("Account 'missing' not found")); - - // Mail Noun Commands: `Sync`/`List`/`Read`/`Search`/`Mark`/`Send` - // (and `Folder::List`) are all real gRPC clients of the `nunciod` - // daemon's `Mail` API -- exactly like `Account::Add`/`List` and - // `System::Status` above, they must never - // run against this `ephemeral()`-constructed runner's production - // `SecretManager` or its (unreachable in CI) default gRPC address. - // They are exercised separately below via `ephemeral_with` + - // `SecretManager::mock()` against a live stub `Mail` gRPC server - // (`mail_rpcs_round_trip_over_grpc_to_a_stub_daemon`). - - // Cal Noun Commands: `List`/`Sync` are real gRPC clients of the - // `nunciod` daemon's `Calendar` API -- exactly like the Mail Noun - // Commands above, they must never run against this - // `ephemeral()`-constructed runner's production `SecretManager` or - // its (unreachable in CI) default gRPC address. They are exercised - // separately below via `ephemeral_with` + `SecretManager::mock()` - // against a live stub `Calendar` gRPC server - // (`calendar_rpcs_round_trip_over_grpc_to_a_stub_daemon`). - - // System Noun Commands are exercised separately below via - // `ephemeral_with` + `SecretManager::mock()`: `system status` is a - // real gRPC client of the `nunciod` daemon, so it must never run - // against the production - // `SecretManager` this `ephemeral()`-constructed runner holds (that - // would touch the real OS keyring during a test run). - } - /// Binds an ephemeral loopback TCP listener, reads back its OS-assigned /// address, then immediately drops the listener so the port is free /// again. Nothing is listening on the returned address, so connecting @@ -2447,7 +2603,9 @@ mod tests { use nuncio_proto::v1::{ AccountConfig as AccountConfigProto, AccountProtocol as AccountProtocolProto, AddAccountRequest, AddAccountResponse, ListAccountsRequest, ListAccountsResponse, - TlsMode as TlsModeProto, + RemoveAccountRequest, RemoveAccountResponse, TestAccountConnectionRequest, + TestAccountConnectionResponse, TlsMode as TlsModeProto, UpdateAccountRequest, + UpdateAccountResponse, }; use std::sync::Mutex; @@ -2479,6 +2637,32 @@ mod tests { Ok(tonic::Response::new(AddAccountResponse { id })) } + async fn update_account( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Ok(tonic::Response::new(UpdateAccountResponse {})) + } + + async fn remove_account( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Ok(tonic::Response::new(RemoveAccountResponse {})) + } + + async fn test_account_connection( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Ok(tonic::Response::new(TestAccountConnectionResponse { + imap_ok: true, + smtp_ok: true, + imap_error: None, + smtp_error: None, + })) + } + async fn list_accounts( &self, _request: tonic::Request, @@ -2584,6 +2768,90 @@ mod tests { // Plain output must show each account's details, not just the count. assert!(list_out_text.contains("stub@nuncio.mx")); assert!(list_out_text.contains("imap.nuncio.mx")); + + // `account show` reads the daemon's ListAccounts result (filtered + // client-side), not the runner's own ephemeral local db. + let show_out = runner + .execute_command( + &Commands::Account { + action: AccountSubcommand::Show { + id: "acct-stub-1".to_string(), + }, + }, + true, + ) + .await; + assert!(show_out.contains("stub@nuncio.mx")); + assert!(show_out.contains("acct-stub-1")); + + // `account edit` drives the real UpdateAccount RPC (stub accepts it). + let edit_out = runner + .execute_command( + &Commands::Account { + action: AccountSubcommand::Edit { + id: "acct-stub-1".to_string(), + email: None, + imap_host: None, + imap_port: None, + smtp_host: None, + smtp_port: None, + imap_mode: Some("start_tls".to_string()), + smtp_mode: None, + }, + }, + true, + ) + .await; + assert!(edit_out.contains(r#""status":"updated""#)); + + // A garbage transport mode is rejected client-side before any RPC. + let bad_edit = runner + .execute_command( + &Commands::Account { + action: AccountSubcommand::Edit { + id: "acct-stub-1".to_string(), + email: None, + imap_host: None, + imap_port: None, + smtp_host: None, + smtp_port: None, + imap_mode: Some("nonsense".to_string()), + smtp_mode: None, + }, + }, + true, + ) + .await; + assert!(bad_edit.contains("invalid tls mode")); + + // `account test` reports the daemon's genuine per-protocol result -- + // never a fabricated "OK (24ms latency)". + let test_out = runner + .execute_command( + &Commands::Account { + action: AccountSubcommand::Test { + id: "acct-stub-1".to_string(), + }, + }, + true, + ) + .await; + assert!(test_out.contains(r#""imap_ok":true"#)); + assert!(test_out.contains(r#""smtp_ok":true"#)); + assert!(!test_out.contains("latency")); + + // `account delete` drives the real RemoveAccount RPC. + let delete_out = runner + .execute_command( + &Commands::Account { + action: AccountSubcommand::Delete { + id: "acct-stub-1".to_string(), + }, + }, + true, + ) + .await; + assert!(delete_out.contains(r#""status":"deleted""#)); } /// Reference-client proof: boots a stub `nuncio.v1.Mail` gRPC server diff --git a/crates/nuncio-mail/src/imap.rs b/crates/nuncio-mail/src/imap.rs index 98382f9..0366e6e 100644 --- a/crates/nuncio-mail/src/imap.rs +++ b/crates/nuncio-mail/src/imap.rs @@ -2,10 +2,14 @@ use async_trait::async_trait; use nuncio_core::model::{Email, Folder}; +use nuncio_core::TlsMode; use std::fmt::Debug; +use std::pin::Pin; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; -use tokio::io::{AsyncRead, AsyncWrite}; +use std::task::{Context, Poll}; +use std::time::Duration; +use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt, ReadBuf}; use tokio::net::TcpStream; use tokio_rustls::client::TlsStream; use tokio_rustls::rustls::pki_types::ServerName; @@ -13,6 +17,12 @@ use tokio_rustls::rustls::{ClientConfig, RootCertStore}; use tokio_rustls::TlsConnector; use tokio_stream::StreamExt; +/// Upper bound on how long a single IMAP transport connect (TCP dial, plus +/// any STARTTLS negotiation and TLS handshake) may take before it is treated +/// as a failure. Without this an unreachable or silently-dropping host could +/// hang a connect -- and therefore a `TestAccountConnection` RPC -- forever. +const CONNECT_TIMEOUT: Duration = Duration::from_secs(15); + use crate::backend::MailBackend; use crate::parser::{MailError, MimeParserAdapter}; @@ -86,20 +96,191 @@ pub async fn connect_tls_stream(host: &str, port: u16) -> Result>), + /// Plain, unencrypted TCP stream. + Plain(TcpStream), +} + +impl AsyncRead for MailStream { + fn poll_read( + self: Pin<&mut Self>, + cx: &mut Context<'_>, + buf: &mut ReadBuf<'_>, + ) -> Poll> { + match self.get_mut() { + MailStream::Tls(s) => Pin::new(s.as_mut()).poll_read(cx, buf), + MailStream::Plain(s) => Pin::new(s).poll_read(cx, buf), + } + } +} + +impl AsyncWrite for MailStream { + fn poll_write( + self: Pin<&mut Self>, + cx: &mut Context<'_>, + buf: &[u8], + ) -> Poll> { + match self.get_mut() { + MailStream::Tls(s) => Pin::new(s.as_mut()).poll_write(cx, buf), + MailStream::Plain(s) => Pin::new(s).poll_write(cx, buf), + } + } + + fn poll_flush(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + match self.get_mut() { + MailStream::Tls(s) => Pin::new(s.as_mut()).poll_flush(cx), + MailStream::Plain(s) => Pin::new(s).poll_flush(cx), + } + } + + fn poll_shutdown(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + match self.get_mut() { + MailStream::Tls(s) => Pin::new(s.as_mut()).poll_shutdown(cx), + MailStream::Plain(s) => Pin::new(s).poll_shutdown(cx), + } + } +} + +/// Establish a plain, unencrypted TCP stream to an IMAP server. Used for +/// [`TlsMode::Plain`] (e.g. a local test/dev port) -- no TLS is negotiated, +/// so credentials travel in the clear; the caller opted into that explicitly. +pub async fn connect_plain_stream(host: &str, port: u16) -> Result { + let target_port = if port == 0 { 143 } else { port }; + let addr = format!("{}:{}", host, target_port); + TcpStream::connect(&addr) + .await + .map_err(|e| MailError::NetworkError(format!("TCP connection to {} failed: {}", addr, e))) +} + +/// Read a single CRLF-terminated line from an IMAP transport stream, byte by +/// byte. Used only during STARTTLS negotiation, where exactly one greeting +/// line and then a small number of response lines are read before the socket +/// is upgraded -- not on the hot fetch path, so the simple unbuffered read is +/// adequate and avoids buffering bytes that would be lost across the upgrade. +async fn read_imap_line(stream: &mut S) -> Result +where + S: AsyncRead + Unpin, +{ + let mut line = Vec::new(); + let mut byte = [0u8; 1]; + loop { + let n = stream + .read(&mut byte) + .await + .map_err(|e| MailError::NetworkError(format!("reading IMAP line failed: {}", e)))?; + if n == 0 { + return Err(MailError::ImapError( + "connection closed before a complete IMAP line was read".to_string(), + )); + } + if byte[0] == b'\n' { + break; + } + if byte[0] != b'\r' { + line.push(byte[0]); + } + } + Ok(String::from_utf8_lossy(&line).into_owned()) +} + +/// Establish a STARTTLS IMAP stream: connect plain TCP, read the server +/// greeting, issue an explicit `STARTTLS` command, confirm the tagged `OK`, +/// then upgrade the SAME socket to TLS via the shared rustls connector. This +/// is the RFC 3501 / RFC 2595 explicit-TLS flow used on the submission port +/// (143). The upgrade reuses [`build_tls_connector`], so certificate +/// validation is identical to implicit TLS. +pub async fn connect_starttls_stream( + host: &str, + port: u16, +) -> Result, MailError> { + let target_port = if port == 0 { 143 } else { port }; + let addr = format!("{}:{}", host, target_port); + let mut tcp = TcpStream::connect(&addr).await.map_err(|e| { + MailError::NetworkError(format!("TCP connection to {} failed: {}", addr, e)) + })?; + + // Server greeting (untagged `* OK ...`); consume it before commanding. + let greeting = read_imap_line(&mut tcp).await?; + if !greeting.starts_with("* OK") && !greeting.starts_with("* PREAUTH") { + return Err(MailError::ImapError(format!( + "unexpected IMAP greeting during STARTTLS negotiation: {}", + greeting + ))); + } + + const TAG: &str = "N1"; + tcp.write_all(format!("{} STARTTLS\r\n", TAG).as_bytes()) + .await + .map_err(|e| MailError::NetworkError(format!("sending STARTTLS failed: {}", e)))?; + + // Read responses until the tagged completion line for our command, + // tolerating any untagged lines the server may interleave first. + let tag_prefix = format!("{} ", TAG); + loop { + let line = read_imap_line(&mut tcp).await?; + if let Some(rest) = line.strip_prefix(&tag_prefix) { + if rest.starts_with("OK") { + break; + } + return Err(MailError::ImapError(format!( + "server refused STARTTLS: {}", + line + ))); + } + } + + let connector = build_tls_connector()?; + let server_name = ServerName::try_from(host.to_string()) + .map_err(|e| MailError::TlsError(format!("invalid TLS server name '{}': {}", host, e)))?; + let tls_stream = connector.connect(server_name, tcp).await.map_err(|e| { + MailError::TlsError(format!( + "TLS handshake with {} after STARTTLS failed: {}", + host, e + )) + })?; + Ok(tls_stream) +} + /// IMAP dual-socket manager maintaining isolated connections for IDLE push and FETCH/STORE queries. pub struct ImapDualSocketManager { server_host: String, server_port: u16, + tls_mode: TlsMode, idle_active: Arc, } impl ImapDualSocketManager { - /// Create a new `ImapDualSocketManager`. + /// Create a new `ImapDualSocketManager` defaulting to implicit TLS. The + /// default port for a `0` port is the implicit-TLS IMAPS port (993). pub fn new(server_host: &str, server_port: u16) -> Self { - let port = if server_port == 0 { 993 } else { server_port }; + Self::with_tls_mode(server_host, server_port, TlsMode::ImplicitTls) + } + + /// Create a new `ImapDualSocketManager` for a specific transport security + /// [`TlsMode`]. A `0` port is resolved to the conventional default for + /// the chosen mode: 993 for implicit TLS, 143 for STARTTLS/plain. + pub fn with_tls_mode(server_host: &str, server_port: u16, tls_mode: TlsMode) -> Self { + let port = if server_port == 0 { + match tls_mode { + TlsMode::ImplicitTls => 993, + TlsMode::StartTls | TlsMode::Plain => 143, + } + } else { + server_port + }; Self { server_host: server_host.to_string(), server_port: port, + tls_mode, idle_active: Arc::new(AtomicBool::new(false)), } } @@ -114,6 +295,11 @@ impl ImapDualSocketManager { self.server_port } + /// Retrieve the configured transport security mode. + pub fn tls_mode(&self) -> TlsMode { + self.tls_mode + } + /// Retrieve the current IDLE socket state. pub fn idle_state(&self) -> IdleSocketState { if self.idle_active.load(Ordering::SeqCst) { @@ -123,18 +309,44 @@ impl ImapDualSocketManager { } } - /// Establish a live TLS socket connection to the target IMAP server. - pub async fn connect_tls_socket(&self) -> Result, MailError> { - connect_tls_stream(&self.server_host, self.server_port).await + /// Establish a live transport socket to the target IMAP server, selecting + /// implicit TLS, STARTTLS, or plain TCP per the configured [`TlsMode`]. + /// Bounded by [`CONNECT_TIMEOUT`] so an unreachable host cannot hang the + /// caller indefinitely. + pub async fn connect_stream(&self) -> Result { + let connect = async { + match self.tls_mode { + TlsMode::ImplicitTls => Ok(MailStream::Tls(Box::new( + connect_tls_stream(&self.server_host, self.server_port).await?, + ))), + TlsMode::StartTls => Ok(MailStream::Tls(Box::new( + connect_starttls_stream(&self.server_host, self.server_port).await?, + ))), + TlsMode::Plain => Ok(MailStream::Plain( + connect_plain_stream(&self.server_host, self.server_port).await?, + )), + } + }; + tokio::time::timeout(CONNECT_TIMEOUT, connect) + .await + .map_err(|_| { + MailError::NetworkError(format!( + "IMAP connect to {}:{} timed out after {}s", + self.server_host, + self.server_port, + CONNECT_TIMEOUT.as_secs() + )) + })? } - /// Establish an authenticated IMAP session over a live TLS socket connection. + /// Establish an authenticated IMAP session over a live transport socket + /// selected by the configured [`TlsMode`]. pub async fn connect_session( &self, username: &str, password: &str, - ) -> Result>, MailError> { - let stream = self.connect_tls_socket().await?; + ) -> Result, MailError> { + let stream = self.connect_stream().await?; let client = async_imap::Client::new(stream); let session = client .login(username, password) @@ -169,7 +381,7 @@ pub struct ImapEngine { } impl ImapEngine { - /// Create a new `ImapEngine`. + /// Create a new `ImapEngine` defaulting to implicit TLS transport. pub fn new(account_id: &str, server_host: &str, server_port: u16) -> Self { Self { socket_manager: ImapDualSocketManager::new(server_host, server_port), @@ -179,22 +391,53 @@ impl ImapEngine { } } - /// Create a new `ImapEngine` with host, port, and authentication credentials. + /// Create a new `ImapEngine` with host, port, transport security + /// [`TlsMode`], and authentication credentials. The `tls_mode` genuinely + /// selects the transport (implicit TLS / STARTTLS / plain) used for every + /// connection this engine opens -- it is not merely recorded. pub fn with_credentials( account_id: &str, server_host: &str, server_port: u16, + tls_mode: TlsMode, username: &str, password: &str, ) -> Self { Self { - socket_manager: ImapDualSocketManager::new(server_host, server_port), + socket_manager: ImapDualSocketManager::with_tls_mode( + server_host, + server_port, + tls_mode, + ), account_id: account_id.to_string(), username: Some(username.to_string()), password: Some(password.to_string()), } } + /// Probe the IMAP endpoint end to end: open a transport connection using + /// the configured [`TlsMode`], authenticate with the engine's + /// credentials, and cleanly log out. Returns `Ok(())` only on a genuine + /// successful login -- any dial, TLS-handshake, or authentication failure + /// surfaces as the real [`MailError`], never a fabricated success. This + /// backs the daemon's `TestAccountConnection` IMAP leg. + pub async fn probe_connection(&self) -> Result<(), MailError> { + let (username, password) = match (&self.username, &self.password) { + (Some(u), Some(p)) => (u.as_str(), p.as_str()), + _ => { + return Err(MailError::AuthError( + "IMAP connection probe requires credentials".to_string(), + )) + } + }; + let mut session = self + .socket_manager + .connect_session(username, password) + .await?; + let _ = session.logout().await; + Ok(()) + } + /// Set authentication credentials for the IMAP engine. pub fn set_credentials(&mut self, username: &str, password: &str) { self.username = Some(username.to_string()); @@ -520,6 +763,7 @@ mod tests { let manager = ImapDualSocketManager::new("mail.kof22.com", 993); assert_eq!(manager.server_host(), "mail.kof22.com"); assert_eq!(manager.server_port(), 993); + assert_eq!(manager.tls_mode(), TlsMode::ImplicitTls); assert_eq!(manager.idle_state(), IdleSocketState::Disconnected); manager.start_idle_listener()?; @@ -546,6 +790,76 @@ mod tests { Ok(()) } + #[tokio::test] + async fn with_tls_mode_records_mode_and_resolves_default_ports() { + // A `0` port resolves to the conventional default for each mode. + let implicit = ImapDualSocketManager::with_tls_mode("host", 0, TlsMode::ImplicitTls); + assert_eq!(implicit.tls_mode(), TlsMode::ImplicitTls); + assert_eq!(implicit.server_port(), 993); + + let starttls = ImapDualSocketManager::with_tls_mode("host", 0, TlsMode::StartTls); + assert_eq!(starttls.tls_mode(), TlsMode::StartTls); + assert_eq!(starttls.server_port(), 143); + + let plain = ImapDualSocketManager::with_tls_mode("host", 0, TlsMode::Plain); + assert_eq!(plain.tls_mode(), TlsMode::Plain); + assert_eq!(plain.server_port(), 143); + + // An explicit non-zero port is always honored verbatim. + let explicit = ImapDualSocketManager::with_tls_mode("host", 2143, TlsMode::StartTls); + assert_eq!(explicit.server_port(), 2143); + } + + #[tokio::test] + async fn connect_stream_plain_connects_to_a_loopback_listener() { + // Plain mode performs a genuine TCP connect with no TLS: against a + // loopback listener the connect succeeds and yields the plain + // transport variant. + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind loopback listener"); + let addr = listener.local_addr().expect("listener addr"); + let accept = tokio::spawn(async move { + let _ = listener.accept().await; + }); + + let manager = + ImapDualSocketManager::with_tls_mode("127.0.0.1", addr.port(), TlsMode::Plain); + let stream = manager + .connect_stream() + .await + .expect("plain connect to loopback listener succeeds"); + assert!(matches!(stream, MailStream::Plain(_))); + let _ = accept.await; + } + + #[tokio::test] + async fn connect_stream_implicit_tls_fails_fast_against_a_closed_port() { + // Implicit TLS against a closed port fails at the TCP dial (connection + // refused) -- a genuine, honest failure, not a fabricated success. + let manager = ImapDualSocketManager::with_tls_mode("127.0.0.1", 1, TlsMode::ImplicitTls); + let err = manager + .connect_stream() + .await + .expect_err("connect to a closed port must fail"); + assert!(matches!( + err, + MailError::NetworkError(_) | MailError::TlsError(_) + )); + } + + #[tokio::test] + async fn probe_connection_requires_credentials() { + // Without credentials the probe reports an honest error rather than + // dialing or fabricating a result. + let engine = ImapEngine::new("acct-1", "127.0.0.1", 993); + let err = engine + .probe_connection() + .await + .expect_err("probe without credentials must fail"); + assert!(matches!(err, MailError::AuthError(_))); + } + #[test] fn tls_config_building() -> Result<(), MailError> { let config = build_tls_config()?; diff --git a/crates/nuncio-mail/src/smtp.rs b/crates/nuncio-mail/src/smtp.rs index 97c1c69..35e592d 100644 --- a/crates/nuncio-mail/src/smtp.rs +++ b/crates/nuncio-mail/src/smtp.rs @@ -9,6 +9,7 @@ use lettre::transport::smtp::client::{Tls, TlsParameters}; use lettre::transport::smtp::Error as LettreSmtpError; use lettre::{AsyncSmtpTransport, AsyncTransport, Tokio1Executor}; use nuncio_core::model::{Attachment, Email}; +use nuncio_core::TlsMode; use crate::backend::{MessageSender, OutboundMessage}; use crate::parser::MailError; @@ -42,12 +43,39 @@ pub struct SmtpTransportEngine { } impl SmtpTransportEngine { - /// Create a new [`SmtpTransportEngine`] with initialized transport client. - pub fn new(host: &str, port: u16, username: &str, password: &str) -> Result { - let transport = Self::build_transport(host, port, username, password)?; + /// Create a new [`SmtpTransportEngine`] with initialized transport client + /// for a specific transport security [`TlsMode`]. + pub fn new( + host: &str, + port: u16, + tls_mode: TlsMode, + username: &str, + password: &str, + ) -> Result { + let transport = Self::build_transport(host, port, tls_mode, username, password)?; Ok(Self { transport }) } + /// Probe the SMTP endpoint: open a real connection to the configured + /// relay (performing the implicit-TLS wrap or STARTTLS upgrade the + /// configured [`TlsMode`] selects) and confirm the server responds. Backs + /// the daemon's `TestAccountConnection` SMTP leg. Returns `Ok(())` only + /// when a connection genuinely succeeded -- a dial/handshake failure + /// surfaces as the real [`MailError`], never a fabricated success. + /// + /// This validates transport reachability and TLS negotiation; SMTP has no + /// standalone authentication probe (credentials are exercised at message + /// submission time), so this deliberately does not claim to verify auth. + pub async fn probe_connection(&self) -> Result<(), MailError> { + match self.transport.test_connection().await { + Ok(true) => Ok(()), + Ok(false) => Err(MailError::TransportFailed( + "SMTP server did not accept the connection probe".to_string(), + )), + Err(e) => Err(MailError::from(e)), + } + } + /// Create a new [`SmtpTransportEngine`] from an existing [`AsyncSmtpTransport`] client. pub fn with_transport(transport: AsyncSmtpTransport) -> Self { Self { transport } @@ -227,33 +255,53 @@ impl SmtpTransportEngine { Ok(()) } - /// Build an [`AsyncSmtpTransport`] client instance. - /// Configures Implicit TLS (`Tls::Wrapper`) for port 465, and STARTTLS (`Tls::Required`) for port 587 or other ports. + /// Build an [`AsyncSmtpTransport`] client instance whose transport + /// security genuinely follows the configured [`TlsMode`]: + /// + /// - [`TlsMode::ImplicitTls`] wraps the socket in TLS immediately + /// (`Tls::Wrapper`), as on the SMTPS port (465). + /// - [`TlsMode::StartTls`] connects in the clear and REQUIRES a STARTTLS + /// upgrade before proceeding (`Tls::Required`), as on the submission + /// port (587); a server without STARTTLS is rejected rather than + /// silently falling back to plaintext. + /// - [`TlsMode::Plain`] performs no TLS at all (`Tls::None`), for a local + /// test/dev relay -- the caller opted into cleartext explicitly. + /// + /// Implicit-TLS and STARTTLS relays are built via `relay(host)` (which + /// defaults to TLS wrapping) / `starttls_relay(host)` respectively; + /// plaintext is built via the non-encrypting `builder_dangerous(host)`. pub fn build_transport( host: &str, port: u16, + tls_mode: TlsMode, username: &str, password: &str, ) -> Result, MailError> { Self::validate_smtp_config(host, port, username)?; let creds = Credentials::new(username.to_string(), password.to_string()); - let tls_params = TlsParameters::builder(host.to_string()) - .build() - .map_err(|e| MailError::TransportFailed(format!("TLS parameters error: {}", e)))?; - let tls = if port == 465 { - Tls::Wrapper(tls_params) - } else { - Tls::Required(tls_params) + let builder = match tls_mode { + TlsMode::ImplicitTls | TlsMode::StartTls => { + let tls_params = TlsParameters::builder(host.to_string()) + .build() + .map_err(|e| { + MailError::TransportFailed(format!("TLS parameters error: {}", e)) + })?; + let tls = match tls_mode { + TlsMode::StartTls => Tls::Required(tls_params), + _ => Tls::Wrapper(tls_params), + }; + AsyncSmtpTransport::::relay(host) + .map_err(|e| { + MailError::TransportFailed(format!("relay configuration error: {}", e)) + })? + .tls(tls) + } + TlsMode::Plain => AsyncSmtpTransport::::builder_dangerous(host), }; - let transport = AsyncSmtpTransport::::relay(host) - .map_err(|e| MailError::TransportFailed(format!("relay configuration error: {}", e)))? - .port(port) - .credentials(creds) - .tls(tls) - .build(); + let transport = builder.port(port).credentials(creds).build(); Ok(transport) } @@ -383,21 +431,59 @@ mod tests { } #[test] - fn build_transport_creates_starttls_client_for_port_587() { - let transport = SmtpTransportEngine::build_transport("smtp.nuncio.mx", 587, "user", "pass"); + fn build_transport_creates_starttls_client() { + let transport = SmtpTransportEngine::build_transport( + "smtp.nuncio.mx", + 587, + TlsMode::StartTls, + "user", + "pass", + ); assert!(transport.is_ok()); } #[test] - fn build_transport_creates_implicit_tls_client_for_port_465() { - let transport = SmtpTransportEngine::build_transport("smtp.nuncio.mx", 465, "user", "pass"); + fn build_transport_creates_implicit_tls_client() { + let transport = SmtpTransportEngine::build_transport( + "smtp.nuncio.mx", + 465, + TlsMode::ImplicitTls, + "user", + "pass", + ); + assert!(transport.is_ok()); + } + + #[test] + fn build_transport_creates_plaintext_client() { + let transport = SmtpTransportEngine::build_transport( + "smtp.nuncio.mx", + 25, + TlsMode::Plain, + "user", + "pass", + ); assert!(transport.is_ok()); } + #[tokio::test] + async fn probe_connection_to_unreachable_server_fails() { + let engine = SmtpTransportEngine::new("127.0.0.1", 1, TlsMode::ImplicitTls, "user", "pass") + .expect("valid config"); + let err = engine + .probe_connection() + .await + .expect_err("probe against port 1 must fail, never fabricate success"); + assert!(matches!( + err, + MailError::TransportFailed(_) | MailError::SmtpFailed(_) + )); + } + #[tokio::test] async fn send_email_to_unreachable_server_fails_with_transport_error() { - let engine = - SmtpTransportEngine::new("127.0.0.1", 1, "user", "pass").expect("valid config"); + let engine = SmtpTransportEngine::new("127.0.0.1", 1, TlsMode::ImplicitTls, "user", "pass") + .expect("valid config"); let email = sample_email(); let err = engine .send_email(&email) @@ -411,7 +497,8 @@ mod tests { #[test] fn engine_constructor_creates_instance() { - let engine = SmtpTransportEngine::new("smtp.nuncio.mx", 465, "user", "pass"); + let engine = + SmtpTransportEngine::new("smtp.nuncio.mx", 465, TlsMode::ImplicitTls, "user", "pass"); assert!(engine.is_ok()); } @@ -506,8 +593,8 @@ mod tests { #[tokio::test] async fn send_message_to_unreachable_server_fails_with_transport_error() { - let engine = - SmtpTransportEngine::new("127.0.0.1", 1, "user", "pass").expect("valid config"); + let engine = SmtpTransportEngine::new("127.0.0.1", 1, TlsMode::ImplicitTls, "user", "pass") + .expect("valid config"); let message = sample_outbound_message(); let err = engine .send_message(&message) @@ -525,8 +612,8 @@ mod tests { // through to the real `send_message` path, not a separate/fabricated // stub -- exercised via the trait object // exactly like production code (`nunciod::send`) uses it. - let engine = - SmtpTransportEngine::new("127.0.0.1", 1, "user", "pass").expect("valid config"); + let engine = SmtpTransportEngine::new("127.0.0.1", 1, TlsMode::ImplicitTls, "user", "pass") + .expect("valid config"); let sender: &dyn MessageSender = &engine; let message = sample_outbound_message(); let err = sender diff --git a/crates/nuncio-proto/proto/nuncio/v1/descriptor.bin b/crates/nuncio-proto/proto/nuncio/v1/descriptor.bin index a19a85f122f23ffaf4b3ffb224104ca76f0d4409..230409000f736815e6c458655140d33b16e9f95d 100644 GIT binary patch delta 698 zcmZ`%O-jQ+6gG|66_hM&z3%br2=G8u=Q%)hF`ybD$mX4ySU z$8M!(4D7QVwCX$<#;68%P%^wWw3gZ-#%xMCB3Z&{M!Y^W5sBH1tbn^I$+**iN;Gy2 zX^g~48fd(H<>14?g-{gw6Gkb)JYp1)%}y@{bhZeVGdXRjMsb(~Y@!CR(4H*MClbhF zu7F&=Ss=0&hzFn~Bu!cBnnfPA?48P%2t;y}ddb;U90S?1S8_&ms0C{yRjxH&05Ie{ z#5XZnd|I7O<<*@396sd^g1Q#3)Ai|;;)waPV_!pxRQ7w@2}KIRQ%hX))DW=?6}4un iG0J5*x6s;{y>s+uo{fz1j=_IoL;k4s;unwoE9)1zkmr#A delta 29 lcmdm&{4SQ6>vil#=Cd4|d$=@YnU*n5-lo26bAhHG2LQG;3pxM* diff --git a/crates/nuncio-proto/proto/nuncio/v1/nuncio.proto b/crates/nuncio-proto/proto/nuncio/v1/nuncio.proto index 0339047..07eba08 100644 --- a/crates/nuncio-proto/proto/nuncio/v1/nuncio.proto +++ b/crates/nuncio-proto/proto/nuncio/v1/nuncio.proto @@ -144,6 +144,30 @@ service Accounts { // ListAccounts returns every persisted account configuration. Password // credentials are NEVER included in this response. rpc ListAccounts(ListAccountsRequest) returns (ListAccountsResponse); + // UpdateAccount overwrites an existing account's persisted configuration. + // The password is optional: when omitted the existing keyring credential + // is left untouched; when present it REPLACES the stored credential in the + // OS keyring vault (never written to SQLite, never echoed back). A password + // rotation that cannot be persisted is rolled back so the vault never + // diverges from the persisted account row. + rpc UpdateAccount(UpdateAccountRequest) returns (UpdateAccountResponse); + // RemoveAccount deletes a persisted account configuration by id and its + // associated keyring credential. + rpc RemoveAccount(RemoveAccountRequest) returns (RemoveAccountResponse); + // TestAccountConnection performs a real, bounded connection probe against + // the account's configured IMAP and SMTP endpoints (honoring each one's + // TLS transport mode) and reports the GENUINE per-protocol outcome -- never + // a fabricated success. The two legs verify DIFFERENT depths, and callers + // must not conflate them: + // - IMAP verifies a full LOGIN: connect, negotiate TLS/STARTTLS, and + // authenticate with the account's keyring credential. + // - SMTP verifies only TRANSPORT REACHABILITY: connect and negotiate + // TLS/STARTTLS. It does NOT perform an SMTP AUTH exchange, so `smtp_ok` + // does not assert the credential is valid for outbound mail (SMTP has + // no standard authentication probe that does not also submit a message). + // A probe that fails carries the real error detail; each leg is bounded by + // a timeout so the RPC cannot hang against an unreachable host. + rpc TestAccountConnection(TestAccountConnectionRequest) returns (TestAccountConnectionResponse); } // Mirrors `nuncio_core::AccountProtocol`. @@ -220,6 +244,57 @@ message ListAccountsResponse { repeated AccountConfig accounts = 1; } +// UpdateAccountRequest carries the full replacement account configuration +// plus an OPTIONAL password. When `password` is absent the account's existing +// keyring credential is preserved; when present it is written to the OS +// keyring vault (under `config.keyring_secret_key`), exactly like +// `AddAccountRequest` -- never persisted to SQLite, never logged, never +// echoed back. +message UpdateAccountRequest { + AccountConfig config = 1; + optional string password = 2; +} + +// UpdateAccountResponse deliberately carries no fields: a successful RPC +// response (as opposed to a `Status` error) is confirmation enough, and the +// caller already holds the updated config it just sent. +message UpdateAccountResponse {} + +// RemoveAccountRequest names the account configuration to delete by id. +message RemoveAccountRequest { + string id = 1; +} + +// RemoveAccountResponse currently carries no fields; a successful RPC +// response is confirmation enough. +message RemoveAccountResponse {} + +// TestAccountConnectionRequest names the persisted account to probe by id. +message TestAccountConnectionRequest { + string id = 1; +} + +// TestAccountConnectionResponse reports the REAL, independent outcome of +// probing each protocol endpoint -- never a fabricated success. `imap_error` +// / `smtp_error` are present only when the corresponding `*_ok` is false and +// carry the genuine failure detail (dial/handshake/auth error text). The two +// `*_ok` flags verify different depths -- see `TestAccountConnection`. +message TestAccountConnectionResponse { + // Whether the IMAP endpoint FULLY authenticated: connect, TLS/STARTTLS + // negotiation, AND login with the account's credential all succeeded. + bool imap_ok = 1; + // Whether the SMTP endpoint was REACHABLE: connect and TLS/STARTTLS + // negotiation succeeded. This is a transport-reachability check only -- it + // does NOT perform an SMTP AUTH exchange, so it does not assert the + // credential is valid for outbound mail. + bool smtp_ok = 2; + // Present only when `imap_ok` is false: the real IMAP login/handshake error. + optional string imap_error = 3; + // Present only when `smtp_ok` is false: the real SMTP transport/handshake + // error. + optional string smtp_error = 4; +} + // Mail exposes the READ path over the daemon's real, persistent message // store (the data that syncs in via `DatabaseEngine::save_email`). // diff --git a/crates/nuncio-store/src/db.rs b/crates/nuncio-store/src/db.rs index 58b1b15..30cc2f8 100644 --- a/crates/nuncio-store/src/db.rs +++ b/crates/nuncio-store/src/db.rs @@ -186,6 +186,29 @@ pub struct WormChainReport { pub first_broken_seq: Option, } +/// Serialize a [`nuncio_core::TlsMode`] to its stable on-disk `accounts` +/// column form. Kept as a bare snake_case token (matching the column's +/// `'implicit_tls'` default) rather than JSON, so a row inserted by the +/// migration default and one written here are byte-identical. +fn tls_mode_to_db(mode: nuncio_core::TlsMode) -> &'static str { + match mode { + nuncio_core::TlsMode::ImplicitTls => "implicit_tls", + nuncio_core::TlsMode::StartTls => "start_tls", + nuncio_core::TlsMode::Plain => "plain", + } +} + +/// Parse a persisted `accounts` TLS-mode column back into +/// [`nuncio_core::TlsMode`]. An unrecognized value falls back to the safe +/// default (implicit TLS) rather than failing the whole account load. +fn tls_mode_from_db(value: &str) -> nuncio_core::TlsMode { + match value { + "start_tls" => nuncio_core::TlsMode::StartTls, + "plain" => nuncio_core::TlsMode::Plain, + _ => nuncio_core::TlsMode::ImplicitTls, + } +} + impl DatabaseEngine { /// Maximum concurrent read/write pool size. pub const MAX_CONNECTIONS: u32 = 16; @@ -439,7 +462,9 @@ impl DatabaseEngine { keyring_secret_key TEXT NOT NULL, sync_interval_secs INTEGER NOT NULL, smtp_host TEXT, - smtp_port INTEGER + smtp_port INTEGER, + imap_tls_mode TEXT NOT NULL DEFAULT 'implicit_tls', + smtp_tls_mode TEXT NOT NULL DEFAULT 'implicit_tls' ); CREATE TABLE IF NOT EXISTS filter_rules ( @@ -631,11 +656,53 @@ impl DatabaseEngine { .map_err(DatabaseError::Query)?; self.ensure_accounts_smtp_columns().await?; + self.ensure_accounts_tls_mode_columns().await?; self.backfill_message_fts().await?; 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. + /// + /// A fresh database already gets these columns from `CREATE TABLE IF NOT + /// EXISTS accounts` above, so on a fresh database this is a no-op. For a + /// pre-existing database file the columns are added with a + /// `'implicit_tls'` default, so every existing row keeps loading + /// successfully and retains the historical behavior (implicit TLS) it was + /// created under. SQLite has no `ADD COLUMN IF NOT EXISTS`, so column + /// presence is checked explicitly via `PRAGMA table_info` first, making + /// this safe to run on every daemon startup. + async fn ensure_accounts_tls_mode_columns(&self) -> Result<(), DatabaseError> { + let existing_columns: Vec = sqlx::query("PRAGMA table_info(accounts)") + .fetch_all(&self.pool) + .await + .map_err(DatabaseError::Query)? + .iter() + .map(|row| row.get::("name")) + .collect(); + + if !existing_columns.iter().any(|c| c == "imap_tls_mode") { + sqlx::query( + "ALTER TABLE accounts ADD COLUMN imap_tls_mode TEXT NOT NULL DEFAULT 'implicit_tls'", + ) + .execute(&self.pool) + .await + .map_err(DatabaseError::Query)?; + } + if !existing_columns.iter().any(|c| c == "smtp_tls_mode") { + sqlx::query( + "ALTER TABLE accounts ADD COLUMN smtp_tls_mode TEXT NOT NULL DEFAULT 'implicit_tls'", + ) + .execute(&self.pool) + .await + .map_err(DatabaseError::Query)?; + } + + Ok(()) + } + /// Additive, backfill-safe migration that adds the /// `smtp_host` / `smtp_port` columns to a pre-existing `accounts` table /// that predates the SMTP account-endpoint feature. @@ -725,8 +792,8 @@ impl DatabaseEngine { sqlx::query( r#" INSERT OR REPLACE INTO accounts - (id, name, email_address, protocol, server_host, server_port, use_tls, keyring_secret_key, sync_interval_secs, smtp_host, smtp_port) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + (id, name, email_address, protocol, server_host, server_port, use_tls, keyring_secret_key, sync_interval_secs, smtp_host, smtp_port, imap_tls_mode, smtp_tls_mode) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) "#, ) .bind(&config.id) @@ -740,6 +807,8 @@ impl DatabaseEngine { .bind(config.sync_interval_secs as i64) .bind(&config.smtp_host) .bind(config.smtp_port as i64) + .bind(tls_mode_to_db(config.imap_tls_mode)) + .bind(tls_mode_to_db(config.smtp_tls_mode)) .execute(&self.pool) .await .map_err(DatabaseError::Query)?; @@ -747,6 +816,27 @@ impl DatabaseEngine { Ok(()) } + /// Fetch a single persisted [`nuncio_core::AccountConfig`] by id, or + /// `None` if no such account row exists. + pub async fn get_account( + &self, + id: &str, + ) -> Result, DatabaseError> { + Ok(self.list_accounts().await?.into_iter().find(|a| a.id == id)) + } + + /// Delete a persisted account configuration by id. Idempotent: deleting a + /// non-existent id is not an error (the post-condition -- "no account row + /// with this id" -- already holds). + pub async fn delete_account(&self, id: &str) -> Result<(), DatabaseError> { + sqlx::query("DELETE FROM accounts WHERE id = ?") + .bind(id) + .execute(&self.pool) + .await + .map_err(DatabaseError::Query)?; + Ok(()) + } + /// Query all saved accounts in SQLite. #[allow(clippy::type_complexity)] pub async fn list_accounts(&self) -> Result, DatabaseError> { @@ -762,9 +852,11 @@ impl DatabaseEngine { i64, Option, Option, + String, + String, )> = sqlx::query_as( r#" - SELECT id, name, email_address, protocol, server_host, server_port, use_tls, keyring_secret_key, sync_interval_secs, smtp_host, smtp_port + SELECT id, name, email_address, protocol, server_host, server_port, use_tls, keyring_secret_key, sync_interval_secs, smtp_host, smtp_port, imap_tls_mode, smtp_tls_mode FROM accounts "#, ) @@ -787,6 +879,8 @@ impl DatabaseEngine { sync_interval_secs, smtp_host, smtp_port, + imap_tls_mode, + smtp_tls_mode, )| { let protocol = serde_json::from_str(&protocol_str) .unwrap_or(nuncio_core::AccountProtocol::ImapSmtp); @@ -811,8 +905,8 @@ impl DatabaseEngine { smtp_host: resolved_smtp_host, smtp_port: resolved_smtp_port, use_tls: use_tls != 0, - imap_tls_mode: nuncio_core::TlsMode::ImplicitTls, - smtp_tls_mode: nuncio_core::TlsMode::ImplicitTls, + imap_tls_mode: tls_mode_from_db(&imap_tls_mode), + smtp_tls_mode: tls_mode_from_db(&smtp_tls_mode), keyring_secret_key, sync_interval_secs: sync_interval_secs as u64, } @@ -2505,6 +2599,103 @@ mod tests { assert_eq!(accounts[0].smtp_port, 465); } + /// Regression proof that the per-protocol TLS transport modes are + /// genuinely persisted and round-tripped -- previously they were + /// validated on add but silently discarded, with `list_accounts` + /// hardcoding both back to `ImplicitTls` regardless of what was saved. + /// Saves an account whose IMAP mode is `StartTls` and SMTP mode is + /// `Plain`, then re-reads it via BOTH `list_accounts` and `get_account` + /// and asserts each distinct mode survived rather than collapsing to the + /// default. + #[tokio::test] + async fn account_tls_modes_survive_save_and_reload() { + let (engine, _dir) = DatabaseEngine::connect_ephemeral().await.unwrap(); + + let acct = nuncio_core::AccountConfig { + id: "acct-tls-1".to_string(), + name: "Mixed TLS Account".to_string(), + email_address: "mixed@nuncio.mx".to_string(), + protocol: nuncio_core::AccountProtocol::ImapSmtp, + server_host: "imap.nuncio.mx".to_string(), + server_port: 143, + smtp_host: "smtp.nuncio.mx".to_string(), + smtp_port: 25, + use_tls: false, + imap_tls_mode: nuncio_core::TlsMode::StartTls, + smtp_tls_mode: nuncio_core::TlsMode::Plain, + keyring_secret_key: "nuncio/acct-tls-1".to_string(), + sync_interval_secs: 60, + }; + + engine.save_account(&acct).await.expect("save succeeds"); + + let listed = engine.list_accounts().await.expect("list succeeds"); + assert_eq!(listed.len(), 1); + assert_eq!(listed[0].imap_tls_mode, nuncio_core::TlsMode::StartTls); + assert_eq!(listed[0].smtp_tls_mode, nuncio_core::TlsMode::Plain); + + let fetched = engine + .get_account("acct-tls-1") + .await + .expect("get succeeds") + .expect("account present"); + assert_eq!(fetched.imap_tls_mode, nuncio_core::TlsMode::StartTls); + assert_eq!(fetched.smtp_tls_mode, nuncio_core::TlsMode::Plain); + assert_eq!(fetched, acct); + } + + /// Proves `get_account` resolves a saved account by id and returns `None` + /// for an unknown id, and that `delete_account` genuinely removes the row + /// (and is idempotent for an already-absent id). + #[tokio::test] + async fn get_and_delete_account_round_trip() { + let (engine, _dir) = DatabaseEngine::connect_ephemeral().await.unwrap(); + + let acct = nuncio_core::AccountConfig { + id: "acct-del-1".to_string(), + name: "Deletable".to_string(), + email_address: "del@nuncio.mx".to_string(), + protocol: nuncio_core::AccountProtocol::ImapSmtp, + server_host: "imap.nuncio.mx".to_string(), + server_port: 993, + smtp_host: "smtp.nuncio.mx".to_string(), + smtp_port: 465, + use_tls: true, + imap_tls_mode: nuncio_core::TlsMode::ImplicitTls, + smtp_tls_mode: nuncio_core::TlsMode::ImplicitTls, + keyring_secret_key: "nuncio/acct-del-1".to_string(), + sync_interval_secs: 60, + }; + engine.save_account(&acct).await.expect("save succeeds"); + + assert!(engine + .get_account("acct-del-1") + .await + .expect("get succeeds") + .is_some()); + assert!(engine + .get_account("acct-missing") + .await + .expect("get succeeds") + .is_none()); + + engine + .delete_account("acct-del-1") + .await + .expect("delete succeeds"); + assert!(engine + .get_account("acct-del-1") + .await + .expect("get succeeds") + .is_none()); + + // Idempotent: deleting an already-absent id is not an error. + engine + .delete_account("acct-del-1") + .await + .expect("second delete is a no-op"); + } + /// Proves the additive `smtp_host`/`smtp_port` /// migration is backfill-safe. Simulates a database created BEFORE this /// feature existed (an `accounts` table with no `smtp_host`/`smtp_port` diff --git a/crates/nunciod/src/grpc.rs b/crates/nunciod/src/grpc.rs index 0a5a308..e2e189e 100644 --- a/crates/nunciod/src/grpc.rs +++ b/crates/nunciod/src/grpc.rs @@ -9,7 +9,7 @@ use nuncio_cal::CalendarBackend; use nuncio_contacts::ContactsBackend; use nuncio_core::{CoreCommand, CoreEvent, EventBus}; use nuncio_filter::{FilterEngine, NsqlParser, NsqlValidator, ValidationOptions}; -use nuncio_mail::{MailBackend, MessageSender}; +use nuncio_mail::{ImapEngine, MailBackend, MessageSender, SmtpTransportEngine}; use nuncio_proto::v1::accounts_server::{Accounts, AccountsServer}; use nuncio_proto::v1::audit_server::{Audit, AuditServer}; use nuncio_proto::v1::calendar_server::{Calendar, CalendarServer}; @@ -38,9 +38,11 @@ use nuncio_proto::v1::{ ListFoldersResponse, ListMessagesRequest, ListMessagesResponse, ListRecordsRequest, ListRecordsResponse, ListRulesRequest, ListRulesResponse, MarkReadRequest, MarkReadResponse, Message as MessageProto, MessageFlagsChanged, MessageSearchHit, PreviewRuleRequest, - PreviewRuleResponse, SearchMessagesRequest, SearchMessagesResponse, SendMessageRequest, - SendMessageResponse, ShuttingDown, SubscribeRequest, SyncCompleted, SyncRequest, SyncResponse, - SyncStarted, TlsMode as TlsModeProto, TriageProgress, TriageRequest, UpdateAvailable, + PreviewRuleResponse, RemoveAccountRequest, RemoveAccountResponse, SearchMessagesRequest, + SearchMessagesResponse, SendMessageRequest, SendMessageResponse, ShuttingDown, + SubscribeRequest, SyncCompleted, SyncRequest, SyncResponse, SyncStarted, + TestAccountConnectionRequest, TestAccountConnectionResponse, TlsMode as TlsModeProto, + TriageProgress, TriageRequest, UpdateAccountRequest, UpdateAccountResponse, UpdateAvailable, UpdateRuleRequest, UpdateRuleResponse, ValidateRuleRequest, ValidateRuleResponse, VerifyChainRequest, VerifyChainResponse, }; @@ -304,6 +306,134 @@ fn map_account_config_from_proto( }) } +/// Outcome of probing a single protocol endpoint (IMAP or SMTP) during a +/// `TestAccountConnection` RPC. `ok` reflects a GENUINE dial/handshake/auth +/// outcome; `error` carries the real failure detail when `ok` is false. +#[derive(Debug, Clone)] +pub struct ProtocolProbe { + /// Whether the probe genuinely succeeded. + pub ok: bool, + /// Real failure detail, present only when `ok` is false. + pub error: Option, +} + +impl ProtocolProbe { + /// A successful probe. + fn ok() -> Self { + Self { + ok: true, + error: None, + } + } + + /// A failed probe carrying its real error text. + fn failed(error: String) -> Self { + Self { + ok: false, + error: Some(error), + } + } +} + +/// The genuine per-protocol result of a `TestAccountConnection` probe. +#[derive(Debug, Clone)] +pub struct AccountConnectionReport { + /// Result of probing the account's IMAP endpoint. + pub imap: ProtocolProbe, + /// Result of probing the account's SMTP endpoint. + pub smtp: ProtocolProbe, +} + +/// Injectable seam backing `TestAccountConnection`'s real per-protocol +/// connection probing. Production uses [`RealAccountConnectionTester`], which +/// performs a genuine bounded IMAP + SMTP dial/handshake using the account's +/// configured endpoints, TLS modes, and keyring credential. A full-daemon +/// offline test injects a stand-in so the RPC can be exercised end to end +/// without touching any live server. +#[tonic::async_trait] +pub trait AccountConnectionTester: Send + Sync { + /// Probe both protocol endpoints for `config`, authenticating with + /// `password`, and report the genuine per-protocol outcome. + async fn probe( + &self, + config: &nuncio_core::AccountConfig, + password: &str, + ) -> AccountConnectionReport; +} + +/// Production [`AccountConnectionTester`]: builds a real +/// [`nuncio_mail::ImapEngine`] / [`nuncio_mail::SmtpTransportEngine`] from the +/// account's configured endpoints, TLS transport modes, and keyring password, +/// and performs a genuine, bounded connection probe against each. Every +/// result is the real dial/handshake/auth outcome -- never fabricated. +struct RealAccountConnectionTester; + +#[tonic::async_trait] +impl AccountConnectionTester for RealAccountConnectionTester { + async fn probe( + &self, + config: &nuncio_core::AccountConfig, + password: &str, + ) -> AccountConnectionReport { + let imap = match config.protocol { + nuncio_core::AccountProtocol::ImapSmtp => { + let engine = ImapEngine::with_credentials( + &config.id, + &config.server_host, + config.server_port, + config.imap_tls_mode, + &config.email_address, + password, + ); + match engine.probe_connection().await { + Ok(()) => ProtocolProbe::ok(), + Err(e) => ProtocolProbe::failed(e.to_string()), + } + } + nuncio_core::AccountProtocol::Jmap => ProtocolProbe::failed( + "IMAP connection probe is not applicable to a JMAP account".to_string(), + ), + }; + + let smtp = match SmtpTransportEngine::new( + &config.smtp_host, + config.smtp_port, + config.smtp_tls_mode, + &config.email_address, + password, + ) { + Ok(engine) => match engine.probe_connection().await { + Ok(()) => ProtocolProbe::ok(), + Err(e) => ProtocolProbe::failed(e.to_string()), + }, + Err(e) => ProtocolProbe::failed(e.to_string()), + }; + + AccountConnectionReport { imap, smtp } + } +} + +/// Overridable engine seam for the `nuncio.v1.Accounts` service, mirroring +/// [`MailEngineOverrides`]'s shape. Production always uses the default (a real +/// [`RealAccountConnectionTester`]); only a full-daemon offline E2E test +/// injects a stand-in tester so `TestAccountConnection` can be driven over the +/// real authenticated gRPC API without any live server. +#[derive(Clone, Default)] +pub struct AccountsEngineOverrides { + /// When `Some`, `TestAccountConnection` probes through this tester instead + /// of building real per-account IMAP/SMTP engines from keyring + /// credentials. + pub connection_tester: Option>, +} + +impl std::fmt::Debug for AccountsEngineOverrides { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("AccountsEngineOverrides") + .field("connection_tester", &self.connection_tester.is_some()) + .finish() + } +} + /// `nuncio.v1.Accounts` gRPC service implementation backed by the daemon's /// live [`DatabaseEngine`] and [`SecretManager`] vault. /// @@ -314,6 +444,58 @@ fn map_account_config_from_proto( struct AccountsGrpcService { db: Arc, secrets: Arc, + connection_tester: Arc, +} + +/// Rotate an account's stored credential to `new_password` and persist the +/// updated `config`, with a rollback that never destroys a working +/// credential. +/// +/// The prior secret (if any) is snapshotted BEFORE the new one is written, so +/// if `save_account` then fails the snapshot is RESTORED -- returning the +/// vault to exactly its pre-call state. This differs deliberately from +/// `add_account`'s rollback, which deletes: an add has no prior secret to +/// preserve, but an update's key may already hold the working credential, and +/// deleting it on a persist failure would leave the still-present account row +/// referencing a missing secret (strictly worse than doing nothing). When +/// there was no prior secret, restore falls back to deleting the just-written +/// one, matching add's behavior for that case. A rollback failure is reported +/// alongside the original persistence error (which stays primary), never in +/// place of it. +async fn rotate_credential_and_persist( + db: &DatabaseEngine, + secrets: &SecretManager, + config: &nuncio_core::AccountConfig, + new_password: &str, +) -> Result<(), Status> { + // Snapshot the prior credential before overwriting it. `None` means the + // key held no secret (or was unreadable), in which case rollback deletes. + let previous = secrets.get_secret(&config.keyring_secret_key).ok(); + + secrets + .set_secret(&config.keyring_secret_key, new_password) + .map_err(|e| Status::internal(format!("failed to store credential in vault: {e}")))?; + + if let Err(e) = db.save_account(config).await { + let mut message = format!("failed to persist account: {e}"); + let rollback = match &previous { + Some(prior) => secrets.set_secret(&config.keyring_secret_key, prior), + None => secrets.delete_secret(&config.keyring_secret_key), + }; + if let Err(rollback_err) = rollback { + tracing::warn!( + keyring_secret_key = %config.keyring_secret_key, + error = %rollback_err, + "failed to roll back keyring secret after update_account persistence failure" + ); + message.push_str(&format!( + " (and failed to roll back the credential: {rollback_err})" + )); + } + return Err(Status::internal(message)); + } + + Ok(()) } #[tonic::async_trait] @@ -386,6 +568,128 @@ impl Accounts for AccountsGrpcService { Ok(Response::new(ListAccountsResponse { accounts })) } + + async fn update_account( + &self, + request: Request, + ) -> Result, Status> { + let req = request.into_inner(); + let proto_config = req + .config + .ok_or_else(|| Status::invalid_argument("config is required"))?; + + let config = map_account_config_from_proto(proto_config)?; + config + .validate() + .map_err(|e| Status::invalid_argument(e.to_string()))?; + + // Updating a non-existent account is a client error, not a silent + // create: reject it so a typo'd id never conjures a new account row. + if self + .db + .get_account(&config.id) + .await + .map_err(|e| Status::internal(format!("failed to look up account: {e}")))? + .is_none() + { + return Err(Status::not_found(format!( + "account '{}' not found", + config.id + ))); + } + + match req.password { + // A password rotation writes the new credential first, then + // persists. Unlike `add_account` (where no prior secret exists, + // so a failed persist rolls back by DELETING the orphan), an + // update's key may already hold the working credential, so a + // failed persist must RESTORE that prior value rather than delete + // it -- deleting would leave the still-present account row + // referencing a missing secret. See + // [`rotate_credential_and_persist`]. + Some(password) if !password.is_empty() => { + rotate_credential_and_persist(&self.db, &self.secrets, &config, &password).await?; + } + // No password change: the existing keyring credential is left + // entirely untouched and only the persisted configuration is + // overwritten. + _ => { + self.db + .save_account(&config) + .await + .map_err(|e| Status::internal(format!("failed to persist account: {e}")))?; + } + } + + Ok(Response::new(UpdateAccountResponse {})) + } + + async fn remove_account( + &self, + request: Request, + ) -> Result, Status> { + let req = request.into_inner(); + if req.id.is_empty() { + return Err(Status::invalid_argument("id is required")); + } + + let existing = self + .db + .get_account(&req.id) + .await + .map_err(|e| Status::internal(format!("failed to look up account: {e}")))? + .ok_or_else(|| Status::not_found(format!("account '{}' not found", req.id)))?; + + self.db + .delete_account(&req.id) + .await + .map_err(|e| Status::internal(format!("failed to remove account: {e}")))?; + + // Best-effort credential cleanup: the account row is already gone, so + // a keyring deletion failure must not fail the RPC (which would + // wrongly imply the account still exists). Log it and move on -- an + // orphaned secret with no account row is inert. + if let Err(e) = self.secrets.delete_secret(&existing.keyring_secret_key) { + tracing::warn!( + keyring_secret_key = %existing.keyring_secret_key, + error = %e, + "failed to delete keyring credential after removing account" + ); + } + + Ok(Response::new(RemoveAccountResponse {})) + } + + async fn test_account_connection( + &self, + request: Request, + ) -> Result, Status> { + let req = request.into_inner(); + if req.id.is_empty() { + return Err(Status::invalid_argument("id is required")); + } + + let config = self + .db + .get_account(&req.id) + .await + .map_err(|e| Status::internal(format!("failed to look up account: {e}")))? + .ok_or_else(|| Status::not_found(format!("account '{}' not found", req.id)))?; + + let password = self + .secrets + .get_secret(&config.keyring_secret_key) + .map_err(|e| Status::internal(format!("failed to read credential from vault: {e}")))?; + + let report = self.connection_tester.probe(&config, &password).await; + + Ok(Response::new(TestAccountConnectionResponse { + imap_ok: report.imap.ok, + smtp_ok: report.smtp.ok, + imap_error: report.imap.error, + smtp_error: report.smtp.error, + })) + } } /// Default cap on messages returned by `ListMessages` when the caller @@ -1850,6 +2154,7 @@ pub async fn serve_on_listener( MailEngineOverrides::default(), CalendarEngineOverrides::default(), ContactsEngineOverrides::default(), + AccountsEngineOverrides::default(), ) .await } @@ -1876,6 +2181,7 @@ pub async fn serve_on_listener_with_overrides( overrides: MailEngineOverrides, calendar_overrides: CalendarEngineOverrides, contacts_overrides: ContactsEngineOverrides, + accounts_overrides: AccountsEngineOverrides, ) -> Result<(), GrpcServeError> { let token: Arc = token.into(); @@ -1888,6 +2194,9 @@ pub async fn serve_on_listener_with_overrides( let accounts_service = AccountsGrpcService { db: db.clone(), secrets: secrets.clone(), + connection_tester: accounts_overrides + .connection_tester + .unwrap_or_else(|| Arc::new(RealAccountConnectionTester)), }; let accounts_interceptor = BearerAuthInterceptor::new(token.clone()); let accounts_svc = AccountsServer::with_interceptor(accounts_service, accounts_interceptor); @@ -2069,6 +2378,36 @@ mod tests { overrides: MailEngineOverrides, calendar_overrides: CalendarEngineOverrides, contacts_overrides: ContactsEngineOverrides, + ) -> (std::net::SocketAddr, tokio::task::JoinHandle<()>) { + spawn_test_server_with_every_override( + event_bus, + db, + filter_engine, + secrets, + token, + overrides, + calendar_overrides, + contacts_overrides, + AccountsEngineOverrides::default(), + ) + .await + } + + /// Widest test spawn helper: threads an [`AccountsEngineOverrides`] too, + /// so a test can inject a stand-in [`AccountConnectionTester`] and drive + /// `TestAccountConnection` over the real authenticated gRPC API without a + /// live server. + #[allow(clippy::too_many_arguments)] + async fn spawn_test_server_with_every_override( + event_bus: Arc, + db: Arc, + filter_engine: Arc, + secrets: Arc, + token: &str, + overrides: MailEngineOverrides, + calendar_overrides: CalendarEngineOverrides, + contacts_overrides: ContactsEngineOverrides, + accounts_overrides: AccountsEngineOverrides, ) -> (std::net::SocketAddr, tokio::task::JoinHandle<()>) { let listener = TcpListener::bind("127.0.0.1:0") .await @@ -2086,6 +2425,7 @@ mod tests { overrides, calendar_overrides, contacts_overrides, + accounts_overrides, ) .await; }); @@ -2771,6 +3111,96 @@ mod tests { ); } + /// Regression: a password rotation that fails to persist must RESTORE the + /// prior credential, never destroy it. Seeds an existing credential, + /// forces `save_account` to fail for real (closed pool), and asserts the + /// ORIGINAL secret is still retrievable afterward -- proving the update + /// rollback restores rather than deletes (which would orphan the still + /// present account row). Drives the exact rotation+rollback helper the + /// `update_account` RPC uses. + #[tokio::test] + async fn update_account_password_rotation_restores_prior_credential_on_persist_failure() { + let (db, _dir) = DatabaseEngine::connect_ephemeral() + .await + .expect("connect ephemeral test db"); + let secrets = SecretManager::mock(); + + let key = "nuncio/acct-rotate-1"; + secrets + .set_secret(key, "original-working-password") + .expect("seed the prior credential"); + + let config = nuncio_core::AccountConfig { + id: "acct-rotate-1".to_string(), + name: "Rotate Account".to_string(), + email_address: "rotate@nuncio.mx".to_string(), + protocol: nuncio_core::AccountProtocol::ImapSmtp, + server_host: "imap.nuncio.mx".to_string(), + server_port: 993, + smtp_host: "smtp.nuncio.mx".to_string(), + smtp_port: 465, + use_tls: true, + imap_tls_mode: nuncio_core::TlsMode::ImplicitTls, + smtp_tls_mode: nuncio_core::TlsMode::ImplicitTls, + keyring_secret_key: key.to_string(), + sync_interval_secs: 60, + }; + + // Force `save_account` to fail for real by closing the pool, exactly + // as `add_account`'s rollback test does. + db.close().await; + + let err = rotate_credential_and_persist(&db, &secrets, &config, "new-rotated-password") + .await + .expect_err("save_account must fail once the pool is closed"); + assert_eq!(err.code(), Code::Internal); + assert!(err.message().contains("failed to persist account")); + + // The prior credential must be intact -- neither deleted nor left as + // the failed new value. + let restored = secrets.get_secret(key).expect("prior credential survives"); + assert_eq!(restored, "original-working-password"); + } + + /// Companion: when the key held NO prior secret, a failed rotation falls + /// back to deleting the just-written one (matching `add_account`), leaving + /// no orphaned credential. + #[tokio::test] + async fn update_account_password_rotation_deletes_new_credential_when_none_existed() { + let (db, _dir) = DatabaseEngine::connect_ephemeral() + .await + .expect("connect ephemeral test db"); + let secrets = SecretManager::mock(); + + let key = "nuncio/acct-rotate-2"; + let config = nuncio_core::AccountConfig { + id: "acct-rotate-2".to_string(), + name: "Rotate Account 2".to_string(), + email_address: "rotate2@nuncio.mx".to_string(), + protocol: nuncio_core::AccountProtocol::ImapSmtp, + server_host: "imap.nuncio.mx".to_string(), + server_port: 993, + smtp_host: "smtp.nuncio.mx".to_string(), + smtp_port: 465, + use_tls: true, + imap_tls_mode: nuncio_core::TlsMode::ImplicitTls, + smtp_tls_mode: nuncio_core::TlsMode::ImplicitTls, + keyring_secret_key: key.to_string(), + sync_interval_secs: 60, + }; + + db.close().await; + + let err = rotate_credential_and_persist(&db, &secrets, &config, "new-rotated-password") + .await + .expect_err("save_account must fail once the pool is closed"); + assert_eq!(err.code(), Code::Internal); + + // No prior secret existed, so the just-written one is rolled back by + // deletion -- nothing orphaned. + assert!(secrets.get_secret(key).is_err()); + } + // ---- Mail ---- use nuncio_proto::v1::mail_client::MailClient; diff --git a/crates/nunciod/src/send.rs b/crates/nunciod/src/send.rs index 1602396..afcd079 100644 --- a/crates/nunciod/src/send.rs +++ b/crates/nunciod/src/send.rs @@ -113,6 +113,7 @@ fn build_smtp_sender( let engine = SmtpTransportEngine::new( &config.smtp_host, config.smtp_port, + config.smtp_tls_mode, &config.email_address, password, )?; diff --git a/crates/nunciod/src/sync.rs b/crates/nunciod/src/sync.rs index d260505..d81c0aa 100644 --- a/crates/nunciod/src/sync.rs +++ b/crates/nunciod/src/sync.rs @@ -317,6 +317,7 @@ fn build_mail_backend(config: &AccountConfig, password: &str) -> Box AccountConnectionReport { + AccountConnectionReport { + imap: ProtocolProbe { + ok: true, + error: None, + }, + smtp: ProtocolProbe { + ok: false, + error: Some("SMTP relay refused the connection".to_string()), + }, + } + } +} + +fn sample_account_config() -> AccountConfig { + AccountConfig { + id: ACCOUNT_ID.to_string(), + name: "Lifecycle E2E Account".to_string(), + email_address: ACCOUNT_EMAIL.to_string(), + protocol: AccountProtocol::ImapSmtp.into(), + server_host: "imap.nuncio.mx".to_string(), + server_port: 143, + use_tls: false, + imap_tls_mode: TlsMode::StartTls.into(), + 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_port: 25, + } +} + +/// Boots the real daemon gRPC server on an ephemeral loopback port with an +/// injected [`AccountsEngineOverrides`]. Returns the server address and the +/// minted token; the `TempDir` guard is returned so the caller controls its +/// lifetime for the whole test. +async fn boot_daemon( + secrets: Arc, + accounts_overrides: AccountsEngineOverrides, +) -> (std::net::SocketAddr, String, tempfile::TempDir) { + let dir = tempfile::tempdir().expect("create temp dir for file-backed db"); + let db_path = dir.path().join("accounts_lifecycle_e2e.db"); + let db = Arc::new( + DatabaseEngine::connect_file(&db_path, &secrets) + .await + .expect("open temp file-backed database"), + ); + + let token_bytes = secrets + .get_or_create_key_bytes(GRPC_TOKEN_ACCOUNT, 32) + .expect("mint gRPC bearer token from mock vault"); + let token = hex::encode(token_bytes); + + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("bind ephemeral loopback port"); + let addr = listener.local_addr().expect("listener has local addr"); + + let event_bus = Arc::new(EventBus::new()); + let server_secrets = secrets.clone(); + let server_token = token.clone(); + let server_filter_engine = Arc::new(FilterEngine::new(Vec::new()).expect("empty rule set")); + tokio::spawn(async move { + let _ = serve_on_listener_with_overrides( + listener, + event_bus, + db, + server_filter_engine, + server_secrets, + server_token, + MailEngineOverrides::default(), + CalendarEngineOverrides::default(), + ContactsEngineOverrides::default(), + accounts_overrides, + ) + .await; + }); + + (addr, token, dir) +} + +#[tokio::test] +async fn account_lifecycle_round_trips_over_authenticated_grpc_offline() { + let secrets = Arc::new(SecretManager::mock()); + let accounts_overrides = AccountsEngineOverrides { + connection_tester: Some(Arc::new(StubConnectionTester)), + }; + let (addr, token, _db_dir) = boot_daemon(secrets, accounts_overrides).await; + let addr_str = addr.to_string(); + + let mut accounts_client = nuncio_proto::client::connect_accounts(&addr_str, &token) + .await + .expect("accounts client connects"); + + // ---- (a) AddAccount persists a StartTls/Plain account ---- + let add_response = accounts_client + .add_account(AddAccountRequest { + config: Some(sample_account_config()), + password: "lifecycle-secret-password".to_string(), + }) + .await + .expect("add_account succeeds") + .into_inner(); + assert_eq!(add_response.id, ACCOUNT_ID); + + // ---- (b) ListAccounts round-trips BOTH distinct TLS modes ---- + let listed = accounts_client + .list_accounts(ListAccountsRequest {}) + .await + .expect("list_accounts succeeds") + .into_inner() + .accounts; + assert_eq!(listed.len(), 1); + assert_eq!(listed[0].imap_tls_mode(), TlsMode::StartTls); + assert_eq!(listed[0].smtp_tls_mode(), TlsMode::Plain); + + // ---- (c) UpdateAccount genuinely mutates the persisted config ---- + let mut updated = sample_account_config(); + updated.name = "Renamed Lifecycle Account".to_string(); + updated.imap_tls_mode = TlsMode::ImplicitTls.into(); + accounts_client + .update_account(UpdateAccountRequest { + config: Some(updated), + password: None, + }) + .await + .expect("update_account succeeds"); + + let relisted = accounts_client + .list_accounts(ListAccountsRequest {}) + .await + .expect("list_accounts succeeds") + .into_inner() + .accounts; + assert_eq!(relisted.len(), 1); + assert_eq!(relisted[0].name, "Renamed Lifecycle Account"); + assert_eq!(relisted[0].imap_tls_mode(), TlsMode::ImplicitTls); + // Untouched fields survive the update. + assert_eq!(relisted[0].smtp_tls_mode(), TlsMode::Plain); + + // ---- (d) TestAccountConnection reports the GENUINE per-protocol result ---- + let report = accounts_client + .test_account_connection(TestAccountConnectionRequest { + id: ACCOUNT_ID.to_string(), + }) + .await + .expect("test_account_connection succeeds") + .into_inner(); + assert!(report.imap_ok); + assert!(!report.smtp_ok); + assert_eq!(report.imap_error, None); + assert_eq!( + report.smtp_error.as_deref(), + Some("SMTP relay refused the connection") + ); + + // ---- (e) RemoveAccount genuinely deletes it ---- + accounts_client + .remove_account(RemoveAccountRequest { + id: ACCOUNT_ID.to_string(), + }) + .await + .expect("remove_account succeeds"); + + let after_remove = accounts_client + .list_accounts(ListAccountsRequest {}) + .await + .expect("list_accounts succeeds") + .into_inner() + .accounts; + assert!(after_remove.is_empty()); + + // A test against the now-removed account is an honest not-found, never a + // fabricated success. + let err = accounts_client + .test_account_connection(TestAccountConnectionRequest { + id: ACCOUNT_ID.to_string(), + }) + .await + .expect_err("testing a removed account must fail"); + assert_eq!(err.code(), tonic::Code::NotFound); +} diff --git a/crates/nunciod/tests/calendar_e2e_test.rs b/crates/nunciod/tests/calendar_e2e_test.rs index d0e90b7..72b534d 100644 --- a/crates/nunciod/tests/calendar_e2e_test.rs +++ b/crates/nunciod/tests/calendar_e2e_test.rs @@ -30,8 +30,8 @@ use nuncio_proto::v1::{CalendarSyncRequest, GetEventRequest, ListEventsRequest}; use nuncio_store::db::DatabaseEngine; use nuncio_store::vault::{SecretManager, GRPC_TOKEN_ACCOUNT}; use nunciod::grpc::{ - serve_on_listener_with_overrides, CalendarEngineOverrides, ContactsEngineOverrides, - MailEngineOverrides, + serve_on_listener_with_overrides, AccountsEngineOverrides, CalendarEngineOverrides, + ContactsEngineOverrides, MailEngineOverrides, }; use std::sync::Arc; use tokio::net::TcpListener; @@ -95,6 +95,7 @@ async fn boot_daemon( MailEngineOverrides::default(), calendar_overrides, ContactsEngineOverrides::default(), + AccountsEngineOverrides::default(), ) .await; }); diff --git a/crates/nunciod/tests/contacts_e2e_test.rs b/crates/nunciod/tests/contacts_e2e_test.rs index b5e9d8f..f231f39 100644 --- a/crates/nunciod/tests/contacts_e2e_test.rs +++ b/crates/nunciod/tests/contacts_e2e_test.rs @@ -39,8 +39,8 @@ use nuncio_proto::v1::{ use nuncio_store::db::DatabaseEngine; use nuncio_store::vault::{SecretManager, GRPC_TOKEN_ACCOUNT}; use nunciod::grpc::{ - serve_on_listener_with_overrides, CalendarEngineOverrides, ContactsEngineOverrides, - MailEngineOverrides, + serve_on_listener_with_overrides, AccountsEngineOverrides, CalendarEngineOverrides, + ContactsEngineOverrides, MailEngineOverrides, }; use std::sync::Arc; use tokio::net::TcpListener; @@ -97,6 +97,7 @@ async fn boot_daemon( MailEngineOverrides::default(), CalendarEngineOverrides::default(), contacts_overrides, + AccountsEngineOverrides::default(), ) .await; }); diff --git a/crates/nunciod/tests/grpc_auth_test.rs b/crates/nunciod/tests/grpc_auth_test.rs index 767e051..22b543e 100644 --- a/crates/nunciod/tests/grpc_auth_test.rs +++ b/crates/nunciod/tests/grpc_auth_test.rs @@ -188,6 +188,54 @@ async fn rejects_calls_to_new_filters_rpcs_without_a_bearer_token() { assert_eq!(err.code(), Code::Unauthenticated); } +/// Confirms the newly added `Accounts` lifecycle RPCs +/// (`UpdateAccount`/`RemoveAccount`/`TestAccountConnection`) are each mounted +/// behind the SAME `BearerAuthInterceptor` as every other service -- an +/// unauthenticated call to any of them must be rejected exactly like an +/// unauthenticated `System/GetStatus`, with no carve-out for "just one more" +/// RPC. +#[tokio::test] +async fn rejects_calls_to_new_accounts_rpcs_without_a_bearer_token() { + let event_bus = Arc::new(EventBus::new()); + let secrets = SecretManager::mock(); + let token = hex::encode( + secrets + .get_or_create_key_bytes(GRPC_TOKEN_ACCOUNT, 32) + .expect("token provisioned from mock vault"), + ); + + let (addr, _dir) = start_server(event_bus, token).await; + let mut client = + nuncio_proto::v1::accounts_client::AccountsClient::connect(format!("http://{addr}")) + .await + .expect("client connects"); + + let update_err = client + .update_account(nuncio_proto::v1::UpdateAccountRequest { + config: None, + password: None, + }) + .await + .expect_err("unauthenticated update_account must be rejected"); + assert_eq!(update_err.code(), Code::Unauthenticated); + + let remove_err = client + .remove_account(nuncio_proto::v1::RemoveAccountRequest { + id: "acct-1".to_string(), + }) + .await + .expect_err("unauthenticated remove_account must be rejected"); + assert_eq!(remove_err.code(), Code::Unauthenticated); + + let test_err = client + .test_account_connection(nuncio_proto::v1::TestAccountConnectionRequest { + id: "acct-1".to_string(), + }) + .await + .expect_err("unauthenticated test_account_connection must be rejected"); + assert_eq!(test_err.code(), Code::Unauthenticated); +} + /// Confirms the streaming `Filters.Triage` RPC is mounted behind the SAME /// `BearerAuthInterceptor` as every other `Filters` RPC -- an unauthenticated /// call must be rejected before the stream is ever established, exactly diff --git a/crates/nunciod/tests/spine_e2e_test.rs b/crates/nunciod/tests/spine_e2e_test.rs index 27427c1..f4d6253 100644 --- a/crates/nunciod/tests/spine_e2e_test.rs +++ b/crates/nunciod/tests/spine_e2e_test.rs @@ -43,8 +43,8 @@ use nuncio_proto::v1::{ use nuncio_store::db::DatabaseEngine; use nuncio_store::vault::{SecretManager, GRPC_TOKEN_ACCOUNT}; use nunciod::grpc::{ - serve_on_listener_with_overrides, CalendarEngineOverrides, ContactsEngineOverrides, - MailEngineOverrides, + serve_on_listener_with_overrides, AccountsEngineOverrides, CalendarEngineOverrides, + ContactsEngineOverrides, MailEngineOverrides, }; use std::sync::Arc; use tokio::net::TcpListener; @@ -130,6 +130,7 @@ async fn boot_daemon( overrides, CalendarEngineOverrides::default(), ContactsEngineOverrides::default(), + AccountsEngineOverrides::default(), ) .await; }); -- 2.54.0 From d7dce525e2b4ed9d43768bfd0d84e811588c334f Mon Sep 17 00:00:00 2001 From: James Maes Date: Wed, 29 Jul 2026 21:33:55 -0500 Subject: [PATCH 3/5] feat(sync): incremental IMAP sync with progress + per-item FETCH timeout (#231) * feat(store): add per-folder sync-state checkpoint table + CRUD Adds folder_sync_state(account_id, folder_id, state) with get/save methods and a round-trip test, giving inbound sync a place to persist a per-mailbox resumption token between runs. Refs #195 #184 * feat(mail): honor since_state for incremental IMAP fetch + bound FETCH stalls ImapEngine now derives a real UIDNEXT-based checkpoint after selecting a folder and narrows the UID FETCH to {uidnext}:* when a checkpoint is supplied, instead of always re-fetching 1:* and returning a state fabricated from emails.len(). An unparseable checkpoint falls back to a full fetch but is surfaced distinctly. Each per-item FETCH read is bounded by a named timeout, surfacing MailError::FetchStalled instead of hanging. MockMailBackend captures the since_state arg and simulates an incremental (empty) second fetch. Session-level duplex tests assert the narrowed FETCH range and the bounded stall. Refs #195 #184 #183 * feat(sync): persist and thread the folder checkpoint, publish sync progress fetch_and_persist now reads the folder's stored checkpoint, passes it to the backend for a narrowed fetch, and persists the returned checkpoint only after that folder's messages land -- so the second sync of an account/folder is genuinely incremental. Adds CoreEvent::SyncProgress (per folder) as an internal signal; map_core_event drops it from the gRPC stream since it has no wire-contract case (proto unchanged). Refs #195 #184 * fix(mail): track UIDVALIDITY in sync checkpoint to prevent mail loss A checkpoint stored only UIDNEXT, so a mailbox renumber (UIDVALIDITY change, UIDs restart low) would drive {old_uidnext}:* against the new low UIDs -- per RFC 3501 n:* returns only the single highest message and silently skips the rest, permanently dropping mail. The checkpoint now encodes {uidvalidity}:{uidnext}; the incremental narrowing applies only when the stored UIDVALIDITY still matches the mailbox's current one. A mismatch (renumber), a missing current UIDVALIDITY, or a legacy bare-number/unparseable checkpoint falls back to a safe full 1:* re-fetch with a debug-visible warn, then rewrites the checkpoint fresh. Refs #195 #184 --------- Co-authored-by: James Maes --- crates/nuncio-core/src/lib.rs | 13 + crates/nuncio-mail/Cargo.toml | 3 + crates/nuncio-mail/src/imap.rs | 552 +++++++++++++++++++++++++++++-- crates/nuncio-mail/src/mock.rs | 54 ++- crates/nuncio-mail/src/parser.rs | 8 + crates/nuncio-store/src/db.rs | 113 +++++++ crates/nunciod/src/grpc.rs | 16 +- crates/nunciod/src/sync.rs | 138 +++++++- 8 files changed, 866 insertions(+), 31 deletions(-) diff --git a/crates/nuncio-core/src/lib.rs b/crates/nuncio-core/src/lib.rs index 0bc6b68..5921251 100644 --- a/crates/nuncio-core/src/lib.rs +++ b/crates/nuncio-core/src/lib.rs @@ -93,6 +93,19 @@ pub enum CoreEvent { /// Action description. action_taken: String, }, + /// Progress notification emitted during an inbound mail sync, so a + /// long-running sync of a large mailbox is observably progressing rather + /// than indistinguishable from a hang. Emitted per folder as it completes. + SyncProgress { + /// Account being synced, if the sync is scoped to one. + account_id: Option, + /// Folder whose messages were just fetched and persisted. + folder_id: String, + /// Messages fetched from this folder on this pass. + fetched: usize, + /// Cumulative messages fetched across the sync so far. + total: usize, + }, /// Progress notification during bulk filter execution over a batch of emails. BatchFilterProgress { /// Messages processed so far in the batch. diff --git a/crates/nuncio-mail/Cargo.toml b/crates/nuncio-mail/Cargo.toml index 7099f8e..644a401 100644 --- a/crates/nuncio-mail/Cargo.toml +++ b/crates/nuncio-mail/Cargo.toml @@ -31,3 +31,6 @@ reqwest.workspace = true [dev-dependencies] tempfile.workspace = true wiremock.workspace = true +# `test-util` (not in tokio's `full`) enables paused-clock tests so the +# bounded per-item FETCH timeout can be asserted without a real-time wait. +tokio = { workspace = true, features = ["test-util"] } diff --git a/crates/nuncio-mail/src/imap.rs b/crates/nuncio-mail/src/imap.rs index 0366e6e..36e8a3c 100644 --- a/crates/nuncio-mail/src/imap.rs +++ b/crates/nuncio-mail/src/imap.rs @@ -23,9 +23,108 @@ use tokio_stream::StreamExt; /// hang a connect -- and therefore a `TestAccountConnection` RPC -- forever. const CONNECT_TIMEOUT: Duration = Duration::from_secs(15); +/// Upper bound on how long a single `FETCH` response item may take to arrive +/// once the `UID FETCH` command has been acknowledged. Fetching full bodies +/// for a large mailbox in one command can be legitimately slow overall, so the +/// bound is per item rather than for the whole stream; but a single item that +/// never arrives (dead socket, server-side hang) must still surface as an +/// honest timeout rather than hanging the sync indefinitely. +const FETCH_ITEM_TIMEOUT: Duration = Duration::from_secs(60); + use crate::backend::MailBackend; use crate::parser::{MailError, MimeParserAdapter}; +/// Delimiter between the two components of a folder sync checkpoint. The +/// checkpoint state string is `"{uidvalidity}{DELIM}{uidnext}"` (e.g. +/// `"42:105"`): the mailbox's UIDVALIDITY at the time it was written, then the +/// UID boundary to resume from. Both components are decimal `u32`. +/// +/// UIDVALIDITY MUST be carried because IMAP UIDs are only meaningful within a +/// stable UIDVALIDITY (RFC 3501 s2.3.1.1). If a mailbox is recreated and its +/// UIDVALIDITY changes, UIDs restart from low numbers, so resuming from a UID +/// boundary captured under the old UIDVALIDITY would issue `{old_uidnext}:*` +/// against renumbered low UIDs -- which per `n:*` semantics returns only the +/// single highest message and silently skips the rest. Comparing UIDVALIDITY +/// first turns that into a safe full re-fetch instead. +const CHECKPOINT_DELIM: char = ':'; + +/// The `UID FETCH` sequence-set to issue for a folder sync, resolved from the +/// stored checkpoint. Kept as a small typed value (rather than collapsing to a +/// bare range string) so the distinct fall-back reasons stay observable: all +/// of `Full`/`CorruptCheckpoint`/`UidValidityChanged` fetch `1:*`, but only the +/// latter two are worth flagging, and tests can assert on which branch fired. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum FetchRange { + /// No prior checkpoint (first-ever sync of this folder): fetch everything. + Full, + /// A checkpoint was stored but could not be parsed (malformed, or a legacy + /// bare-UID checkpoint predating UIDVALIDITY tracking). Falls back to a + /// full fetch, surfaced (logged) rather than silently ignored. + CorruptCheckpoint, + /// The stored checkpoint's UIDVALIDITY no longer matches the mailbox's + /// current UIDVALIDITY (the mailbox was renumbered) or the server did not + /// report a current UIDVALIDITY to compare against. Stored UIDs are no + /// longer meaningful, so this falls back to a safe full re-fetch. + UidValidityChanged { + /// UIDVALIDITY recorded in the stored checkpoint. + stored: u32, + /// UIDVALIDITY the server reports now, if any. + current: Option, + }, + /// A valid checkpoint whose UIDVALIDITY still matches: fetch only UIDs at + /// or above the stored boundary, i.e. messages that arrived since. + Incremental(u32), +} + +impl FetchRange { + /// Render the IMAP `UID FETCH` sequence-set. Every non-incremental variant + /// fetches all UIDs (`1:*`); an incremental checkpoint `n` fetches `n:*`, + /// which the server bounds to UIDs `>= n` -- a genuinely narrower fetch. + fn sequence_set(self) -> String { + match self { + FetchRange::Full + | FetchRange::CorruptCheckpoint + | FetchRange::UidValidityChanged { .. } => "1:*".to_string(), + FetchRange::Incremental(uid) => format!("{}:*", uid), + } + } +} + +/// Resolve the fetch range from a stored `since_state` checkpoint and the +/// mailbox's CURRENT UIDVALIDITY (as just reported by `SELECT`). `None` means +/// no prior sync (full fetch). A checkpoint in the `"{uidvalidity}:{uidnext}"` +/// format proceeds incrementally only when its UIDVALIDITY still matches the +/// server's current one; a mismatch (mailbox renumbered) or a missing current +/// UIDVALIDITY forces a safe full re-fetch. A malformed or legacy bare-number +/// checkpoint is treated as corrupt (full fetch, flagged), never misread as a +/// live UID boundary. +fn resolve_fetch_range(since_state: Option<&str>, current_uid_validity: Option) -> FetchRange { + let Some(raw) = since_state else { + return FetchRange::Full; + }; + let parsed = raw + .split_once(CHECKPOINT_DELIM) + .and_then(|(validity, uid)| { + Some(( + validity.trim().parse::().ok()?, + uid.trim().parse::().ok()?, + )) + }); + let Some((stored_validity, uid)) = parsed else { + return FetchRange::CorruptCheckpoint; + }; + if uid < 1 { + return FetchRange::CorruptCheckpoint; + } + match current_uid_validity { + Some(current) if current == stored_validity => FetchRange::Incremental(uid), + current => FetchRange::UidValidityChanged { + stored: stored_validity, + current, + }, + } +} + /// State of the dedicated IMAP IDLE socket listener. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum IdleSocketState { @@ -449,21 +548,53 @@ impl ImapEngine { &self.socket_manager } - /// Fetch and parse all messages in a folder over an active IMAP session. + /// Fetch and parse the messages in a folder over an active IMAP session, + /// returning the fetched messages and the new sync checkpoint. + /// + /// `since_state` is the checkpoint returned by a previous sync of this + /// folder, encoded as `"{uidvalidity}:{uidnext}"` (see [`CHECKPOINT_DELIM`]). + /// The `UID FETCH` is narrowed to `{uidnext}:*` only when the checkpoint's + /// UIDVALIDITY still matches the mailbox's current UIDVALIDITY; a mismatch + /// (the mailbox was renumbered) or a malformed/legacy checkpoint forces a + /// safe full re-fetch instead of silently skipping renumbered mail. The + /// returned checkpoint is derived from real server state -- the mailbox's + /// UIDVALIDITY plus its `UIDNEXT` (or one past the highest UID actually + /// seen) after selecting the folder -- never from a synthetic counter. pub async fn sync_folder_messages_with_session( &self, folder_id: &str, + since_state: Option<&str>, session: &mut async_imap::Session, - ) -> Result, MailError> + ) -> Result<(Vec, String), MailError> where S: AsyncRead + AsyncWrite + Unpin + Send + Debug + 'static, { - session.select(folder_id).await.map_err(|e| { + let mailbox = session.select(folder_id).await.map_err(|e| { MailError::ImapError(format!("failed to select folder '{}': {}", folder_id, e)) })?; + let server_uid_next = mailbox.uid_next; + let server_uid_validity = mailbox.uid_validity; + + let range = resolve_fetch_range(since_state, server_uid_validity); + match range { + FetchRange::CorruptCheckpoint => tracing::warn!( + folder_id, + checkpoint = since_state.unwrap_or_default(), + "ignoring unparseable folder sync checkpoint; falling back to a full fetch" + ), + FetchRange::UidValidityChanged { stored, current } => tracing::warn!( + folder_id, + stored_uidvalidity = stored, + current_uidvalidity = current, + "mailbox UIDVALIDITY changed (renumbered); the stored UID checkpoint is no \ + longer valid, so re-fetching in full to avoid silently skipping messages" + ), + FetchRange::Full | FetchRange::Incremental(_) => {} + } let query = build_fetch_command_query(); - let mut fetch_stream = session.uid_fetch("1:*", query).await.map_err(|e| { + let sequence_set = range.sequence_set(); + let mut fetch_stream = session.uid_fetch(&sequence_set, query).await.map_err(|e| { MailError::ImapError(format!( "UID FETCH failed for folder '{}': {}", folder_id, e @@ -471,11 +602,29 @@ impl ImapEngine { })?; let mut emails = Vec::new(); - while let Some(fetch_res) = fetch_stream.next().await { + let mut max_uid_seen: u32 = 0; + loop { + // Bound EVERY individual item read: a slow-but-progressing large + // fetch is fine, but a single item that never arrives must surface + // as an honest `FetchStalled` instead of hanging the whole sync. + let next_item = tokio::time::timeout(FETCH_ITEM_TIMEOUT, fetch_stream.next()) + .await + .map_err(|_| { + MailError::FetchStalled(format!( + "no FETCH response for folder '{}' within {}s ({} message(s) read so far)", + folder_id, + FETCH_ITEM_TIMEOUT.as_secs(), + emails.len() + )) + })?; + let Some(fetch_res) = next_item else { + break; + }; let fetch_data = fetch_res .map_err(|e| MailError::ImapError(format!("failed reading fetch item: {}", e)))?; 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); let is_read = fetch_data @@ -559,14 +708,44 @@ impl ImapEngine { emails.push(email); } - Ok(emails) + // Derive the next checkpoint from real server state as + // `"{uidvalidity}:{uidnext}"`. The UID boundary is the mailbox's + // authoritative UIDNEXT, else one past the highest UID actually seen. + // A checkpoint is only meaningful when both a UIDVALIDITY and a UID + // boundary are known; if either is missing (a server that omitted + // UIDVALIDITY, or an empty fetch that revealed no UIDNEXT/UID), keep + // the prior checkpoint rather than writing a validity-less one that the + // next sync could misread. As a last resort (no prior checkpoint + // either) emit an intentionally unparseable marker so the next sync + // safely falls back to a full fetch. + let boundary_uid = server_uid_next + .or_else(|| (max_uid_seen > 0).then_some(max_uid_seen.saturating_add(1))); + let new_checkpoint = match (server_uid_validity, boundary_uid) { + (Some(validity), Some(uid)) => format!("{}{}{}", validity, CHECKPOINT_DELIM, uid), + _ => since_state + .map(str::to_string) + .unwrap_or_else(|| "full-resync-required".to_string()), + }; + + Ok((emails, new_checkpoint)) } - /// Execute `UID FETCH 1:* (FLAGS INTERNALDATE RFC822.SIZE ENVELOPE BODY.PEEK[])` and parse returned messages into domain `Email` entities. - pub async fn sync_folder_messages(&self, folder_id: &str) -> Result, MailError> { + /// Fetch a folder's messages over a freshly authenticated session, + /// honoring `since_state` for an incremental fetch and returning the new + /// sync checkpoint alongside the messages. See + /// [`Self::sync_folder_messages_with_session`] for the checkpoint model. + pub async fn sync_folder_messages( + &self, + folder_id: &str, + since_state: Option<&str>, + ) -> Result<(Vec, String), MailError> { let (username, password) = match (&self.username, &self.password) { (Some(u), Some(p)) => (u.as_str(), p.as_str()), _ => { + // Credential-less fixture path used by unit tests that never + // reach a real server. The checkpoint (one past the fixture's + // UID) is internally consistent with the fixture rather than + // derived from `emails.len()`. let mock_emails = vec![Email { id: "imap-uid-100".to_string(), account_id: self.account_id.clone(), @@ -580,7 +759,7 @@ impl ImapEngine { body_html: None, attachments: Vec::new(), }]; - return Ok(mock_emails); + return Ok((mock_emails, "101".to_string())); } }; @@ -588,11 +767,11 @@ impl ImapEngine { .socket_manager .connect_session(username, password) .await?; - let emails = self - .sync_folder_messages_with_session(folder_id, &mut session) - .await?; + let result = self + .sync_folder_messages_with_session(folder_id, since_state, &mut session) + .await; let _ = session.logout().await; - Ok(emails) + result } /// Listen for real-time IMAP IDLE notification events on an active IMAP session. @@ -742,11 +921,9 @@ impl MailBackend for ImapEngine { async fn sync_messages( &self, folder_id: &str, - _since_state: Option<&str>, + since_state: Option<&str>, ) -> Result<(Vec, String), MailError> { - let emails = self.sync_folder_messages(folder_id).await?; - let modseq = format!("imap-modseq-{}", emails.len()); - Ok((emails, modseq)) + self.sync_folder_messages(folder_id, since_state).await } async fn send_email(&self, _email: &Email) -> Result<(), MailError> { @@ -781,8 +958,9 @@ mod tests { assert_eq!(folders.len(), 2); assert_eq!(folders[0].id, "INBOX"); - let (emails, modseq) = engine.sync_messages("INBOX", None).await?; - assert_eq!(modseq, "imap-modseq-1"); + let (emails, checkpoint) = engine.sync_messages("INBOX", None).await?; + // Credential-less fixture: checkpoint is one past the fixture UID. + assert_eq!(checkpoint, "101"); assert_eq!(emails.len(), 1); assert_eq!(emails[0].id, "imap-uid-100"); @@ -790,6 +968,342 @@ mod tests { Ok(()) } + #[test] + fn resolve_fetch_range_distinguishes_none_valid_corrupt_and_renumbered_checkpoints() { + // First-ever sync: full history (current UIDVALIDITY irrelevant). + assert_eq!(resolve_fetch_range(None, Some(1)), FetchRange::Full); + assert_eq!(resolve_fetch_range(None, None).sequence_set(), "1:*"); + + // Valid checkpoint whose UIDVALIDITY still matches: narrowed fetch. + assert_eq!( + resolve_fetch_range(Some("1:105"), Some(1)), + FetchRange::Incremental(105) + ); + assert_eq!( + resolve_fetch_range(Some("1:105"), Some(1)).sequence_set(), + "105:*" + ); + assert_eq!( + resolve_fetch_range(Some(" 7 : 42 "), Some(7)), + FetchRange::Incremental(42) + ); + + // UIDVALIDITY changed (mailbox renumbered) or the server reported none + // to compare against: MUST fall back to a full fetch, distinctly, so a + // renumber can never silently skip the low new UIDs. + assert_eq!( + resolve_fetch_range(Some("1:105"), Some(2)), + FetchRange::UidValidityChanged { + stored: 1, + current: Some(2) + } + ); + assert_eq!( + resolve_fetch_range(Some("1:105"), Some(2)).sequence_set(), + "1:*" + ); + assert_eq!( + resolve_fetch_range(Some("1:105"), None), + FetchRange::UidValidityChanged { + stored: 1, + current: None + } + ); + + // Malformed or legacy bare-number (pre-UIDVALIDITY) checkpoints are + // corrupt -> full fetch, never misread as a live UID boundary. + assert_eq!( + resolve_fetch_range(Some(""), Some(1)), + FetchRange::CorruptCheckpoint + ); + assert_eq!( + resolve_fetch_range(Some("105"), Some(1)), + FetchRange::CorruptCheckpoint, + "a legacy bare-number checkpoint must not be read as a UID boundary" + ); + assert_eq!( + resolve_fetch_range(Some("1:0"), Some(1)), + FetchRange::CorruptCheckpoint + ); + assert_eq!( + resolve_fetch_range(Some("not:auid"), Some(1)), + FetchRange::CorruptCheckpoint + ); + assert_eq!( + resolve_fetch_range(Some("not-a-uid"), Some(1)).sequence_set(), + "1:*" + ); + } + + /// Read a single CRLF-terminated line from a scripted-server stream, + /// returning `None` at EOF (client hung up). Used only by the in-memory + /// duplex tests below; the real fetch path never parses raw lines. + async fn read_scripted_line(reader: &mut R) -> Option { + let mut buf = Vec::new(); + let mut byte = [0u8; 1]; + loop { + match reader.read(&mut byte).await { + Ok(0) => { + return if buf.is_empty() { + None + } else { + Some(String::from_utf8_lossy(&buf).into_owned()) + } + } + Ok(_) => { + if byte[0] == b'\n' { + return Some(String::from_utf8_lossy(&buf).into_owned()); + } + if byte[0] != b'\r' { + buf.push(byte[0]); + } + } + Err(_) => return None, + } + } + } + + #[tokio::test] + async fn incremental_sync_narrows_the_fetch_range_on_the_second_pass() { + // Drives a real `async_imap::Session` over an in-memory duplex pair + // whose server side is a scripted IMAP responder. Proves the SECOND + // sync issues a GENUINELY narrower `UID FETCH` derived from the first + // sync's returned checkpoint -- not a full re-fetch. + let (client_io, server_io) = tokio::io::duplex(8192); + let fetch_ranges = Arc::new(std::sync::Mutex::new(Vec::::new())); + let server_ranges = fetch_ranges.clone(); + + let server = tokio::spawn(async move { + let mut io = server_io; + while let Some(line) = read_scripted_line(&mut io).await { + let tag = line.split_whitespace().next().unwrap_or("").to_string(); + let upper = line.to_ascii_uppercase(); + if upper.contains("UID FETCH") { + if let Ok(mut g) = server_ranges.lock() { + g.push(line.clone()); + } + let _ = io + .write_all(format!("{tag} OK FETCH completed\r\n").as_bytes()) + .await; + } else if upper.contains("LOGIN") { + let _ = io + .write_all(format!("{tag} OK Logged in\r\n").as_bytes()) + .await; + } else if upper.contains("SELECT") { + let resp = format!( + "* FLAGS (\\Seen)\r\n* 3 EXISTS\r\n* 0 RECENT\r\n\ + * OK [UIDVALIDITY 1] ok\r\n* OK [UIDNEXT 105] ok\r\n\ + {tag} OK [READ-WRITE] SELECT done\r\n" + ); + let _ = io.write_all(resp.as_bytes()).await; + } else if upper.contains("LOGOUT") { + let _ = io + .write_all(format!("* BYE\r\n{tag} OK LOGOUT\r\n").as_bytes()) + .await; + break; + } + } + }); + + let client = async_imap::Client::new(client_io); + let mut session = client + .login("user", "pass") + .await + .map_err(|(e, _)| e) + .expect("scripted login succeeds"); + + let engine = ImapEngine::new("acct-1", "example.test", 993); + + // First sync: no checkpoint -> full history; returns + // "{uidvalidity}:{uidnext}" from real server state. + let (first, checkpoint) = engine + .sync_folder_messages_with_session("INBOX", None, &mut session) + .await + .expect("first sync succeeds"); + assert!(first.is_empty()); + assert_eq!(checkpoint, "1:105"); + + // Second sync: feed back the checkpoint; UIDVALIDITY still matches + // (1), so the fetch is narrowed. + let (_second, checkpoint2) = engine + .sync_folder_messages_with_session("INBOX", Some(&checkpoint), &mut session) + .await + .expect("second sync succeeds"); + assert_eq!(checkpoint2, "1:105"); + + drop(session); + let _ = server.await; + + let ranges = fetch_ranges.lock().expect("lock captured ranges"); + assert_eq!(ranges.len(), 2, "expected exactly two UID FETCH commands"); + assert!( + ranges[0].contains("1:*"), + "first sync must fetch full history, got: {}", + ranges[0] + ); + assert!( + ranges[1].contains("105:*"), + "second sync must be narrowed to the checkpoint, got: {}", + ranges[1] + ); + assert!( + !ranges[1].contains("1:*"), + "second sync must NOT re-fetch full history, got: {}", + ranges[1] + ); + } + + #[tokio::test] + async fn a_uidvalidity_change_forces_a_full_resync_instead_of_skipping_mail() { + // A mailbox renumber changes UIDVALIDITY and restarts UIDs from low + // numbers. Resuming from the old UID boundary (`105:*`) would return + // only the single highest message and silently drop the rest, so a + // UIDVALIDITY mismatch MUST force a full `1:*` re-fetch. The scripted + // server reports UIDVALIDITY 1 on the first SELECT and 2 on the second. + let (client_io, server_io) = tokio::io::duplex(8192); + let fetch_ranges = Arc::new(std::sync::Mutex::new(Vec::::new())); + let server_ranges = fetch_ranges.clone(); + + let server = tokio::spawn(async move { + let mut io = server_io; + let mut select_count = 0u32; + while let Some(line) = read_scripted_line(&mut io).await { + let tag = line.split_whitespace().next().unwrap_or("").to_string(); + let upper = line.to_ascii_uppercase(); + if upper.contains("UID FETCH") { + if let Ok(mut g) = server_ranges.lock() { + g.push(line.clone()); + } + let _ = io + .write_all(format!("{tag} OK FETCH completed\r\n").as_bytes()) + .await; + } else if upper.contains("LOGIN") { + let _ = io + .write_all(format!("{tag} OK Logged in\r\n").as_bytes()) + .await; + } else if upper.contains("SELECT") { + select_count += 1; + // First SELECT: UIDVALIDITY 1. Second: a DIFFERENT + // UIDVALIDITY (2), i.e. the mailbox was renumbered. + let uidvalidity = if select_count == 1 { 1 } else { 2 }; + let resp = format!( + "* FLAGS (\\Seen)\r\n* 3 EXISTS\r\n* 0 RECENT\r\n\ + * OK [UIDVALIDITY {uidvalidity}] ok\r\n* OK [UIDNEXT 105] ok\r\n\ + {tag} OK [READ-WRITE] SELECT done\r\n" + ); + let _ = io.write_all(resp.as_bytes()).await; + } else if upper.contains("LOGOUT") { + let _ = io + .write_all(format!("* BYE\r\n{tag} OK LOGOUT\r\n").as_bytes()) + .await; + break; + } + } + }); + + let client = async_imap::Client::new(client_io); + let mut session = client + .login("user", "pass") + .await + .map_err(|(e, _)| e) + .expect("scripted login succeeds"); + + let engine = ImapEngine::new("acct-1", "example.test", 993); + + // First sync under UIDVALIDITY 1 stores "1:105". + let (_first, checkpoint) = engine + .sync_folder_messages_with_session("INBOX", None, &mut session) + .await + .expect("first sync succeeds"); + assert_eq!(checkpoint, "1:105"); + + // Second sync: server now reports UIDVALIDITY 2. The stored checkpoint + // is stale, so the fetch MUST be a full `1:*`, and the checkpoint is + // rewritten under the new UIDVALIDITY. + let (_second, checkpoint2) = engine + .sync_folder_messages_with_session("INBOX", Some(&checkpoint), &mut session) + .await + .expect("second sync succeeds"); + assert_eq!( + checkpoint2, "2:105", + "checkpoint must be rewritten under the new UIDVALIDITY" + ); + + drop(session); + let _ = server.await; + + let ranges = fetch_ranges.lock().expect("lock captured ranges"); + assert_eq!(ranges.len(), 2, "expected exactly two UID FETCH commands"); + assert!( + ranges[0].contains("1:*"), + "first sync fetches full history, got: {}", + ranges[0] + ); + assert!( + ranges[1].contains("1:*") && !ranges[1].contains("105:*"), + "a UIDVALIDITY change MUST force a full re-fetch (1:*), never 105:*, got: {}", + ranges[1] + ); + } + + #[tokio::test(start_paused = true)] + async fn a_never_responding_fetch_surfaces_fetchstalled_without_hanging() { + // The scripted server answers LOGIN and SELECT but then goes silent on + // the FETCH. With the tokio clock paused, the bounded per-item timeout + // fast-forwards, so the stall is turned into an honest `FetchStalled` + // error in negligible WALL-CLOCK time -- proving it never hangs. + let (client_io, server_io) = tokio::io::duplex(8192); + + let server = tokio::spawn(async move { + let mut io = server_io; + while let Some(line) = read_scripted_line(&mut io).await { + let tag = line.split_whitespace().next().unwrap_or("").to_string(); + let upper = line.to_ascii_uppercase(); + if upper.contains("UID FETCH") { + // Deliberately never respond: a mid-stream server stall. + std::future::pending::<()>().await; + } else if upper.contains("LOGIN") { + let _ = io + .write_all(format!("{tag} OK Logged in\r\n").as_bytes()) + .await; + } else if upper.contains("SELECT") { + let resp = format!( + "* 1 EXISTS\r\n* OK [UIDVALIDITY 1] ok\r\n* OK [UIDNEXT 42] ok\r\n\ + {tag} OK [READ-WRITE] SELECT done\r\n" + ); + let _ = io.write_all(resp.as_bytes()).await; + } + } + }); + + let client = async_imap::Client::new(client_io); + let mut session = client + .login("user", "pass") + .await + .map_err(|(e, _)| e) + .expect("scripted login succeeds"); + let engine = ImapEngine::new("acct-1", "example.test", 993); + + // Real wall clock, not the (paused) tokio clock: proves negligible + // elapsed time despite the 60s virtual timeout expiring. + let started = std::time::Instant::now(); + let err = engine + .sync_folder_messages_with_session("INBOX", None, &mut session) + .await + .expect_err("a never-responding FETCH must surface an error"); + assert!( + matches!(err, MailError::FetchStalled(_)), + "expected FetchStalled, got: {err:?}" + ); + assert!( + started.elapsed() < Duration::from_secs(5), + "stalled fetch must not hang in wall-clock time, took {:?}", + started.elapsed() + ); + + server.abort(); + } + #[tokio::test] async fn with_tls_mode_records_mode_and_resolves_default_ports() { // A `0` port resolves to the conventional default for each mode. diff --git a/crates/nuncio-mail/src/mock.rs b/crates/nuncio-mail/src/mock.rs index 8c2ec27..d2352fe 100644 --- a/crates/nuncio-mail/src/mock.rs +++ b/crates/nuncio-mail/src/mock.rs @@ -14,12 +14,41 @@ pub struct MockMailBackend { messages: Arc>>, sent_messages: Arc>>, should_fail: Arc>, + /// Every `since_state` argument this mock's `sync_messages` was called + /// with, in call order, so tests can prove a caller threads the returned + /// checkpoint back into the next sync (real incrementality). + since_state_calls: Arc>>>, + /// The checkpoint token this mock returns from `sync_messages`. + returned_state: Arc>, } impl MockMailBackend { /// Create a new `MockMailBackend` with empty storage. pub fn new() -> Self { - Self::default() + let backend = Self::default(); + if let Ok(mut state) = backend.returned_state.lock() { + *state = "mock-state-token-100".to_string(); + } + backend + } + + /// Override the checkpoint token this mock returns from `sync_messages`, + /// so a test can control the exact state a caller must persist and thread + /// back on the next sync. + pub fn set_returned_state(&self, state: &str) { + if let Ok(mut guard) = self.returned_state.lock() { + *guard = state.to_string(); + } + } + + /// Every `since_state` argument passed to `sync_messages`, in call order. + /// A caller that genuinely resumes from a checkpoint will show `None` on + /// the first call and the previously-returned state on the next. + pub fn since_state_calls(&self) -> Vec> { + self.since_state_calls + .lock() + .map(|guard| guard.clone()) + .unwrap_or_default() } /// Configure the mock to simulate network failure errors. @@ -74,8 +103,14 @@ impl MailBackend for MockMailBackend { async fn sync_messages( &self, folder_id: &str, - _since_state: Option<&str>, + since_state: Option<&str>, ) -> Result<(Vec, String), MailError> { + // Record the checkpoint arg before any early return, so even a failing + // call is visible to a test asserting on how the caller resumes. + if let Ok(mut calls) = self.since_state_calls.lock() { + calls.push(since_state.map(str::to_string)); + } + let should_fail = self .should_fail .lock() @@ -85,6 +120,19 @@ impl MailBackend for MockMailBackend { "simulated network failure".to_string(), )); } + let returned_state = self + .returned_state + .lock() + .map_err(|e| MailError::ParseFailed(e.to_string()))? + .clone(); + + // Simulate a since_state-aware backend: with a prior checkpoint there + // is nothing new to hand back, so the incremental fetch is genuinely + // narrower (empty) rather than a full re-report of every message. + if since_state.is_some() { + return Ok((Vec::new(), returned_state)); + } + let messages = self .messages .lock() @@ -94,7 +142,7 @@ impl MailBackend for MockMailBackend { .filter(|m| m.folder_id == folder_id) .cloned() .collect(); - Ok((matches, "mock-state-token-100".to_string())) + Ok((matches, returned_state)) } async fn send_email(&self, email: &Email) -> Result<(), MailError> { diff --git a/crates/nuncio-mail/src/parser.rs b/crates/nuncio-mail/src/parser.rs index 3f7c896..28976b9 100644 --- a/crates/nuncio-mail/src/parser.rs +++ b/crates/nuncio-mail/src/parser.rs @@ -32,6 +32,14 @@ pub enum MailError { #[error("IMAP error: {0}")] ImapError(String), + /// A single FETCH response item exceeded its bounded read timeout without + /// the server responding. Distinct from a generic [`MailError::ImapError`] + /// so callers can tell an honest mid-stream stall (dead socket, server-side + /// hang) apart from a protocol/parse failure instead of the sync hanging + /// forever. + #[error("IMAP FETCH stalled: {0}")] + FetchStalled(String), + /// Network or IO error. #[error("network I/O error: {0}")] NetworkError(String), diff --git a/crates/nuncio-store/src/db.rs b/crates/nuncio-store/src/db.rs index 30cc2f8..5f43173 100644 --- a/crates/nuncio-store/src/db.rs +++ b/crates/nuncio-store/src/db.rs @@ -649,6 +649,19 @@ impl DatabaseEngine { INSERT INTO contacts_fts(id, display_name, organization, emails_json) VALUES (new.id, new.display_name, COALESCE(new.organization, ''), new.emails_json); END; + + -- Per-folder inbound-sync checkpoint. `state` is an opaque, + -- protocol-defined resumption token (an IMAP UID boundary such as + -- UIDNEXT, a JMAP state string) that a backend hands back after a + -- sync and expects to receive on the next sync so it can fetch only + -- what changed since. Keyed by (account, folder) because the token + -- is meaningful only within a single mailbox on a single account. + CREATE TABLE IF NOT EXISTS folder_sync_state ( + account_id TEXT NOT NULL, + folder_id TEXT NOT NULL, + state TEXT NOT NULL, + PRIMARY KEY (account_id, folder_id) + ); "#, ) .execute(&self.pool) @@ -915,6 +928,51 @@ impl DatabaseEngine { .collect()) } + /// Fetch the persisted inbound-sync checkpoint for a folder, or `None` if + /// this account/folder pair has never completed a sync. A `None` result is + /// the signal to the backend that it must perform a full first-time fetch. + pub async fn get_folder_sync_state( + &self, + account_id: &str, + folder_id: &str, + ) -> Result, DatabaseError> { + let row: Option<(String,)> = sqlx::query_as( + "SELECT state FROM folder_sync_state WHERE account_id = ? AND folder_id = ?", + ) + .bind(account_id) + .bind(folder_id) + .fetch_optional(&self.pool) + .await + .map_err(DatabaseError::Query)?; + Ok(row.map(|(state,)| state)) + } + + /// Persist the inbound-sync checkpoint a backend returned for a folder, + /// overwriting any prior value (a checkpoint is a running high-water mark, + /// not history). Written only after a folder's messages have been fetched + /// and persisted, so the stored checkpoint can never advance past work that + /// actually landed in the store. + pub async fn save_folder_sync_state( + &self, + account_id: &str, + folder_id: &str, + state: &str, + ) -> Result<(), DatabaseError> { + sqlx::query( + r#" + INSERT OR REPLACE INTO folder_sync_state (account_id, folder_id, state) + VALUES (?, ?, ?) + "#, + ) + .bind(account_id) + .bind(folder_id) + .bind(state) + .execute(&self.pool) + .await + .map_err(DatabaseError::Query)?; + Ok(()) + } + /// Save an [`nuncio_core::model::Email`] to SQLite (INSERT OR REPLACE). /// /// The message body is encrypted (AES-256-GCM) before being written to the `messages` @@ -2696,6 +2754,61 @@ mod tests { .expect("second delete is a no-op"); } + #[tokio::test] + async fn folder_sync_state_round_trip_is_scoped_and_overwrites() { + let (engine, _dir) = DatabaseEngine::connect_ephemeral().await.unwrap(); + + // Absent before any sync completes. + assert_eq!( + engine + .get_folder_sync_state("acct-1", "INBOX") + .await + .expect("get succeeds"), + None + ); + + engine + .save_folder_sync_state("acct-1", "INBOX", "105") + .await + .expect("save succeeds"); + assert_eq!( + engine + .get_folder_sync_state("acct-1", "INBOX") + .await + .expect("get succeeds"), + Some("105".to_string()) + ); + + // A later checkpoint overwrites (high-water mark, not history). + engine + .save_folder_sync_state("acct-1", "INBOX", "220") + .await + .expect("save succeeds"); + assert_eq!( + engine + .get_folder_sync_state("acct-1", "INBOX") + .await + .expect("get succeeds"), + Some("220".to_string()) + ); + + // Keyed by (account, folder): a different folder/account is unaffected. + assert_eq!( + engine + .get_folder_sync_state("acct-1", "Sent") + .await + .expect("get succeeds"), + None + ); + assert_eq!( + engine + .get_folder_sync_state("acct-2", "INBOX") + .await + .expect("get succeeds"), + None + ); + } + /// Proves the additive `smtp_host`/`smtp_port` /// migration is backfill-safe. Simulates a database created BEFORE this /// feature existed (an `accounts` table with no `smtp_host`/`smtp_port` diff --git a/crates/nunciod/src/grpc.rs b/crates/nunciod/src/grpc.rs index e2e189e..97083d4 100644 --- a/crates/nunciod/src/grpc.rs +++ b/crates/nunciod/src/grpc.rs @@ -87,15 +87,17 @@ pub enum GrpcServeError { } /// Maps a `nuncio_core::CoreEvent` domain event onto its wire-format -/// `nuncio.v1.Event` representation, faithfully carrying every variant's +/// `nuncio.v1.Event` representation, carrying every wire-visible variant's /// fields into the matching `oneof` case (see `proto/nuncio/v1/nuncio.proto` -/// for the wire contract). +/// for the wire contract). Returns `None` for events that are internal-only +/// and have no case in the versioned contract, so they are dropped from the +/// gRPC subscription stream rather than forced onto the wire. /// /// `usize` fields (`processed`, `total`, `matched`, `salvaged_rules_count`) /// are narrowed to `u64` for the wire; these are in-process counters that /// cannot realistically approach `u64::MAX`, and protobuf has no native /// `usize` type. -fn map_core_event(event: CoreEvent) -> Event { +fn map_core_event(event: CoreEvent) -> Option { let kind = match event { CoreEvent::SyncStarted { account_id } => Kind::SyncStarted(SyncStarted { account_id }), CoreEvent::SyncCompleted { account_id } => { @@ -140,8 +142,12 @@ fn map_core_event(event: CoreEvent) -> Event { }), CoreEvent::Error { message } => Kind::Error(EventError { message }), CoreEvent::ShuttingDown => Kind::ShuttingDown(ShuttingDown {}), + // Internal sync-progress signal: intentionally not forwarded to gRPC + // subscribers, as it has no case in the versioned wire contract. It is + // observed in-process (e.g. by the daemon's own subscribers) only. + CoreEvent::SyncProgress { .. } => return None, }; - Event { kind: Some(kind) } + Some(Event { kind: Some(kind) }) } /// `nuncio.v1.System` gRPC service implementation backed by the daemon's live @@ -191,7 +197,7 @@ impl System for SystemGrpcService { ) -> Result, Status> { let receiver = self.event_bus.subscribe_events(); let mapped = BroadcastStream::new(receiver).filter_map(|item| match item { - Ok(core_event) => Some(Ok(map_core_event(core_event))), + Ok(core_event) => map_core_event(core_event).map(Ok), Err(BroadcastStreamRecvError::Lagged(skipped)) => { tracing::warn!( "gRPC Subscribe stream lagged behind the daemon event bus; skipped {} \ diff --git a/crates/nunciod/src/sync.rs b/crates/nunciod/src/sync.rs index d81c0aa..a3c4522 100644 --- a/crates/nunciod/src/sync.rs +++ b/crates/nunciod/src/sync.rs @@ -18,7 +18,7 @@ //! only places that resolve a real account's credentials and construct a //! real backend; they then delegate the actual fetch/persist/event-emission //! work to [`sync_with_backend`]. -use nuncio_core::{AccountConfig, AccountProtocol, CoreCommand, EventBus}; +use nuncio_core::{AccountConfig, AccountProtocol, CoreCommand, CoreEvent, EventBus}; use nuncio_filter::{FilterEngine, OutboxManager, RuleAction}; use nuncio_mail::{ImapEngine, JmapEngine, MailBackend, MailError}; use nuncio_store::db::DatabaseError; @@ -235,13 +235,27 @@ async fn is_new_message(db: &nuncio_store::db::DatabaseEngine, message_id: &str) /// [`MockMailBackend`]. async fn fetch_and_persist( db: &nuncio_store::db::DatabaseEngine, + event_bus: &EventBus, backend: &dyn MailBackend, filter_engine: &FilterEngine, + account_id: Option<&str>, ) -> Result { let folders = backend.sync_folders().await?; let mut synced = 0usize; for folder in folders { - let (emails, _state) = backend.sync_messages(&folder.id, None).await?; + // Resume from the folder's stored checkpoint so the backend can fetch + // only what changed since the last sync. A checkpoint is keyed by + // (account, folder), so a run with no account context (a mock-driven + // `SyncAll` with `account_id == None`) has nowhere to key it and always + // performs a full fetch. + let last_state = match account_id { + Some(acct) => db.get_folder_sync_state(acct, &folder.id).await?, + None => None, + }; + let (emails, new_state) = backend + .sync_messages(&folder.id, last_state.as_deref()) + .await?; + let fetched = emails.len(); for email in emails { let is_new = is_new_message(db, &email.id).await; db.save_email(&email).await?; @@ -250,6 +264,19 @@ async fn fetch_and_persist( } synced += 1; } + // Persist the returned checkpoint only AFTER this folder's messages + // have landed, so the stored high-water mark can never advance past + // work that actually reached the store. + if let Some(acct) = account_id { + db.save_folder_sync_state(acct, &folder.id, &new_state) + .await?; + } + event_bus.publish_event(CoreEvent::SyncProgress { + account_id: account_id.map(str::to_string), + folder_id: folder.id.clone(), + fetched, + total: synced, + }); } Ok(synced) } @@ -282,7 +309,8 @@ pub async fn sync_with_backend( None => event_bus.process_command(CoreCommand::SyncAll), } - let result = fetch_and_persist(db, backend, filter_engine).await; + let result = + fetch_and_persist(db, event_bus, backend, filter_engine, account_id.as_deref()).await; if let Err(e) = &result { event_bus.process_command(CoreCommand::ReportError { @@ -394,7 +422,15 @@ pub async fn run_all_accounts_sync( match secrets.get_secret(&config.keyring_secret_key) { Ok(password) => { let backend = build_mail_backend(&config, &password); - match fetch_and_persist(db, backend.as_ref(), filter_engine).await { + match fetch_and_persist( + db, + event_bus, + backend.as_ref(), + filter_engine, + Some(&config.id), + ) + .await + { Ok(count) => total_synced += count, Err(e) => { event_bus.process_command(CoreCommand::ReportError { @@ -619,6 +655,16 @@ mod tests { account_id: Some("acct-mock-1".to_string()) } ); + // A per-folder progress event is published between start and complete. + assert_eq!( + events.recv().await.expect("progress event"), + CoreEvent::SyncProgress { + account_id: Some("acct-mock-1".to_string()), + folder_id: "inbox".to_string(), + fetched: 2, + total: 2, + } + ); assert_eq!( events.recv().await.expect("complete event"), CoreEvent::SyncCompleted { @@ -654,6 +700,16 @@ mod tests { events.recv().await.expect("start event"), CoreEvent::SyncStarted { account_id: None } ); + // The folder is empty, but a progress event still marks it done. + assert_eq!( + events.recv().await.expect("progress event"), + CoreEvent::SyncProgress { + account_id: None, + folder_id: "inbox".to_string(), + fetched: 0, + total: 0, + } + ); assert_eq!( events.recv().await.expect("complete event"), CoreEvent::SyncCompleted { account_id: None } @@ -791,6 +847,15 @@ mod tests { account_id: Some("acct-jmap-real-1".to_string()) } ); + assert_eq!( + events.recv().await.expect("progress event"), + CoreEvent::SyncProgress { + account_id: Some("acct-jmap-real-1".to_string()), + folder_id: "jmap-inbox".to_string(), + fetched: 1, + total: 1, + } + ); assert_eq!( events.recv().await.expect("complete event"), CoreEvent::SyncCompleted { @@ -1202,4 +1267,69 @@ mod tests { "no new outbox mutation should be enqueued on a repeat sync" ); } + + #[tokio::test] + async fn sync_with_backend_persists_and_threads_the_folder_checkpoint_across_syncs() { + // The regression this story exists to kill: the returned checkpoint + // must be persisted and fed back into the NEXT sync, so the second + // sync is genuinely incremental rather than a full re-fetch. Proven by + // capturing the exact `since_state` the backend received on each call. + let (db, _dir) = DatabaseEngine::connect_ephemeral() + .await + .expect("ephemeral db"); + let event_bus = EventBus::new(); + + let mock = MockMailBackend::new(); + mock.set_returned_state("uidnext-4242"); + mock.add_folder(Folder { + id: "inbox".to_string(), + name: "Inbox".to_string(), + total_messages: 1, + unread_messages: 1, + }); + mock.add_message(mock_email("m-inc-1", "inbox", "First")); + + let filter_engine = empty_filter_engine(); + + // First sync: no checkpoint exists yet, so the backend is called with + // `None` and does a full fetch (one message). + let first = sync_with_backend( + &db, + &event_bus, + &mock, + &filter_engine, + Some("acct-inc-1".to_string()), + ) + .await + .expect("first sync succeeds"); + assert_eq!(first, 1); + + // The returned checkpoint must now be persisted for (account, folder). + assert_eq!( + db.get_folder_sync_state("acct-inc-1", "inbox") + .await + .expect("read checkpoint"), + Some("uidnext-4242".to_string()) + ); + + // Second sync: the daemon must feed the stored checkpoint back in. The + // mock returns nothing new for a non-None checkpoint -> narrower fetch. + let second = sync_with_backend( + &db, + &event_bus, + &mock, + &filter_engine, + Some("acct-inc-1".to_string()), + ) + .await + .expect("second sync succeeds"); + assert_eq!(second, 0, "an incremental second sync fetches nothing new"); + + // The proof of genuine incrementality: the backend saw `None` first, + // then the exact checkpoint the first sync returned. + assert_eq!( + mock.since_state_calls(), + vec![None, Some("uidnext-4242".to_string())], + ); + } } -- 2.54.0 From 4b73d11754320c97a83ffd2223cce1c465f283bc Mon Sep 17 00:00:00 2001 From: James Maes Date: Wed, 29 Jul 2026 21:45:16 -0500 Subject: [PATCH 4/5] feat(mail): select an explicit account for SendMessage (#232) SendMessage previously always sent from the first configured account with no way to choose, so it silently used the wrong identity the moment a second account existed. Add an optional account_id field to SendMessageRequest; when set, resolve that specific account and fail with a new AccountNotFound error if it does not exist, rather than falling back to a different account. Omitting account_id preserves the prior first-account behavior. Thread the selector through ComposeRequest, resolve_sending_account, the CLI's mail send --account flag, and regenerate the nuncio.v1 descriptor golden. Refs #196, #170 Co-authored-by: James Maes --- crates/nuncio-cli/src/args.rs | 4 + crates/nuncio-cli/src/runner.rs | 42 +++++- .../proto/nuncio/v1/descriptor.bin | Bin 12734 -> 12785 bytes .../nuncio-proto/proto/nuncio/v1/nuncio.proto | 10 +- crates/nunciod/src/grpc.rs | 4 + crates/nunciod/src/send.rs | 128 +++++++++++++++--- crates/nunciod/tests/spine_e2e_test.rs | 1 + 7 files changed, 170 insertions(+), 19 deletions(-) diff --git a/crates/nuncio-cli/src/args.rs b/crates/nuncio-cli/src/args.rs index 96bba71..77a3314 100644 --- a/crates/nuncio-cli/src/args.rs +++ b/crates/nuncio-cli/src/args.rs @@ -308,6 +308,10 @@ pub enum MailSubcommand { /// Message body text content. #[arg(short, long, help = "Message body text content")] body: String, + /// Identifier of the account to send from (defaults to the first + /// configured account when omitted). + #[arg(short, long, help = "Identifier of the account to send from")] + account: Option, }, /// Full-text search across all cached email messages. Search { diff --git a/crates/nuncio-cli/src/runner.rs b/crates/nuncio-cli/src/runner.rs index 68c34f3..e1fbb93 100644 --- a/crates/nuncio-cli/src/runner.rs +++ b/crates/nuncio-cli/src/runner.rs @@ -322,8 +322,14 @@ impl HeadlessRunner { MailSubcommand::Sync => self.handle_sync(json_mode).await, MailSubcommand::List { folder } => self.handle_list_folder(folder, json_mode).await, MailSubcommand::Read { id } => self.handle_read_message(id, json_mode).await, - MailSubcommand::Send { to, subject, body } => { - self.handle_send_email(to, subject, body, json_mode).await + MailSubcommand::Send { + to, + subject, + body, + account, + } => { + self.handle_send_email(to, subject, body, account.as_deref(), json_mode) + .await } MailSubcommand::Search { query } => self.handle_search(query, json_mode).await, MailSubcommand::Mark { id, read, unread } => { @@ -695,6 +701,7 @@ impl HeadlessRunner { to: &str, subject: &str, body: &str, + account: Option<&str>, json_mode: bool, ) -> String { let mut client = match self.connect_mail_client().await { @@ -710,6 +717,7 @@ impl HeadlessRunner { body_text: body.to_string(), body_html: None, attachments: Vec::new(), + account_id: account.map(str::to_string), }) .await { @@ -3165,6 +3173,7 @@ mod tests { to: "alice@nuncio.mx".to_string(), subject: "Quarterly Roadmap".to_string(), body: "Let's discuss the roadmap.".to_string(), + account: None, }, }, true, @@ -3182,6 +3191,35 @@ mod tests { assert_eq!(recorded_send.to, "alice@nuncio.mx"); assert_eq!(recorded_send.subject, "Quarterly Roadmap"); assert_eq!(recorded_send.body_text, "Let's discuss the roadmap."); + assert_eq!(recorded_send.account_id, None); + + // `mail send --account ` threads the explicit selector through + // to the daemon over the wire, proving the CLI flag is not merely + // parsed but actually reaches `SendMessageRequest.account_id`. + let mail_send_with_account = runner + .execute_command( + &Commands::Mail { + action: MailSubcommand::Send { + to: "alice@nuncio.mx".to_string(), + subject: "Quarterly Roadmap".to_string(), + body: "Let's discuss the roadmap.".to_string(), + account: Some("acct-work".to_string()), + }, + }, + true, + ) + .await; + assert!(mail_send_with_account.contains(r#""sent":true"#)); + + let recorded_send_with_account = send_probe + .lock() + .unwrap_or_else(|e| e.into_inner()) + .clone() + .expect("stub daemon received a second send_message request"); + assert_eq!( + recorded_send_with_account.account_id, + Some("acct-work".to_string()) + ); } /// Reference-client proof: boots a stub `nuncio.v1.Calendar` gRPC server diff --git a/crates/nuncio-proto/proto/nuncio/v1/descriptor.bin b/crates/nuncio-proto/proto/nuncio/v1/descriptor.bin index 230409000f736815e6c458655140d33b16e9f95d..609afaa324768328d9cf368d61bb8edd7bc550b8 100644 GIT binary patch delta 96 zcmdm&{4tq{>)l4CJbtFhOp}ZFPkRU{ad9OkC+C;umBeSJNU$p~YH)fm1#v>fJX1Ot n8J$?UnB$X^op`vo;*;`ID&sRsa&w$`xwzvIIyVOi, /// File attachments. pub attachments: Vec, + /// Identifier of the account to send from. `None` preserves the prior + /// "first configured account" behavior. + pub account_id: Option, } /// Send `message` through `sender`. Pure seam shared by production and @@ -121,14 +131,24 @@ fn build_smtp_sender( } /// Resolves the account to send mail from. See the module-level "Account -/// selection" doc above for why this is currently the first configured -/// account rather than an explicit per-request choice. -async fn resolve_sending_account(db: &DatabaseEngine) -> Result { +/// selection" doc above: an explicit `account_id` must match a configured +/// account or the request fails; `None` preserves the "first configured +/// account" behavior. +async fn resolve_sending_account( + db: &DatabaseEngine, + account_id: Option<&str>, +) -> Result { let accounts = db.list_accounts().await?; - accounts - .into_iter() - .next() - .ok_or(SendError::NoAccountConfigured) + match account_id { + Some(id) => accounts + .into_iter() + .find(|a| a.id == id) + .ok_or_else(|| SendError::AccountNotFound(id.to_string())), + None => accounts + .into_iter() + .next() + .ok_or(SendError::NoAccountConfigured), + } } /// Generates a daemon-local identifier for a sent message, for reference in @@ -154,7 +174,7 @@ pub async fn send_message_for_account( secrets: &SecretManager, request: ComposeRequest, ) -> Result { - let config = resolve_sending_account(db).await?; + let config = resolve_sending_account(db, request.account_id.as_deref()).await?; let password = secrets.get_secret(&config.keyring_secret_key)?; let sender = build_smtp_sender(&config, &password)?; @@ -186,7 +206,7 @@ pub async fn send_message_with_injected_sender( sender: &dyn MessageSender, request: ComposeRequest, ) -> Result { - let config = resolve_sending_account(db).await?; + let config = resolve_sending_account(db, request.account_id.as_deref()).await?; let outbound = OutboundMessage { from: config.email_address, @@ -286,6 +306,7 @@ mod tests { body_text: Some("Body".to_string()), body_html: None, attachments: Vec::new(), + account_id: None, }, ) .await @@ -314,6 +335,7 @@ mod tests { body_text: Some("Body".to_string()), body_html: None, attachments: Vec::new(), + account_id: None, }, ) .await @@ -352,6 +374,7 @@ mod tests { body_text: Some("Body".to_string()), body_html: None, attachments: Vec::new(), + account_id: None, }, ) .await @@ -392,6 +415,7 @@ mod tests { body_text: Some("Body via injected sender.".to_string()), body_html: None, attachments: Vec::new(), + account_id: None, }, ) .await @@ -423,6 +447,7 @@ mod tests { body_text: Some("Body".to_string()), body_html: None, attachments: Vec::new(), + account_id: None, }, ) .await @@ -430,4 +455,75 @@ mod tests { assert!(matches!(err, SendError::NoAccountConfigured)); assert!(mock.sent_messages().is_empty()); } + + /// Proves that with multiple configured accounts, an explicit + /// `account_id` selects that SPECIFIC account as the sender -- not + /// whichever account happens to be first -- by asserting the captured + /// outbound message's `From:` matches the requested account. + #[tokio::test] + async fn send_message_with_injected_sender_uses_the_explicitly_selected_account() { + let (db, _dir) = DatabaseEngine::connect_ephemeral() + .await + .expect("ephemeral db"); + + let first = sample_account("acct-first"); + let second = sample_account("acct-second"); + db.save_account(&first).await.expect("save first account"); + db.save_account(&second).await.expect("save second account"); + + let mock = MockMessageSender::new(); + send_message_with_injected_sender( + &db, + &mock, + ComposeRequest { + to: "bob@nuncio.mx".to_string(), + cc: None, + subject: "Hi".to_string(), + body_text: Some("Body".to_string()), + body_html: None, + attachments: Vec::new(), + account_id: Some(second.id.clone()), + }, + ) + .await + .expect("send succeeds via explicitly selected account"); + + let sent = mock.sent_messages(); + assert_eq!(sent.len(), 1); + assert_eq!(sent[0].from, second.email_address); + assert_ne!(sent[0].from, first.email_address); + } + + /// An `account_id` naming an account that does not exist must fail with + /// the specific [`SendError::AccountNotFound`] variant -- never a silent + /// fallback to the first configured account, and never confusable with + /// [`SendError::NoAccountConfigured`] (which means zero accounts exist). + #[tokio::test] + async fn send_message_with_injected_sender_rejects_unknown_account_id() { + let (db, _dir) = DatabaseEngine::connect_ephemeral() + .await + .expect("ephemeral db"); + + let config = sample_account("acct-only"); + db.save_account(&config).await.expect("save account"); + + let mock = MockMessageSender::new(); + let err = send_message_with_injected_sender( + &db, + &mock, + ComposeRequest { + to: "bob@nuncio.mx".to_string(), + cc: None, + subject: "Hi".to_string(), + body_text: Some("Body".to_string()), + body_html: None, + attachments: Vec::new(), + account_id: Some("acct-does-not-exist".to_string()), + }, + ) + .await + .expect_err("unknown account id must fail"); + assert!(matches!(err, SendError::AccountNotFound(id) if id == "acct-does-not-exist")); + assert!(mock.sent_messages().is_empty()); + } } diff --git a/crates/nunciod/tests/spine_e2e_test.rs b/crates/nunciod/tests/spine_e2e_test.rs index f4d6253..4a2c844 100644 --- a/crates/nunciod/tests/spine_e2e_test.rs +++ b/crates/nunciod/tests/spine_e2e_test.rs @@ -305,6 +305,7 @@ async fn full_mail_spine_round_trips_over_authenticated_grpc_with_mocked_protoco body_text: "Let's discuss the roadmap.".to_string(), body_html: None, attachments: Vec::new(), + account_id: None, }) .await .expect("send_message succeeds") -- 2.54.0 From c8446a5800474af9fedaaff4c02daa70479e9d3c Mon Sep 17 00:00:00 2001 From: James Maes Date: Wed, 29 Jul 2026 21:51:43 -0500 Subject: [PATCH 5/5] docs: mark M5 complete, set M6 as next (#234) * docs: mark M5 complete (feature surface done), set M6 as next * docs: correct HANDOFF service count to eight --------- Co-authored-by: James Maes --- docs/HANDOFF.md | 46 ++++++++++++++++++++++++----------------- docs/ROADMAP.md | 35 +++++++++++++++++++++---------- docs/roadmap/index.html | 32 ++++++++++++++++------------ 3 files changed, 70 insertions(+), 43 deletions(-) diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index 48e3ba9..c6dbec5 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -25,22 +25,30 @@ machine (or a new LLM) picks up the work cold. ## Current state of the code (2026-07-30) - **Phases 0–2 complete.** The daemon boots gRPC-only on loopback - `127.0.0.1:9420` behind a keyring bearer token, serving six services (System, - Accounts, Mail, Filters, Export, Audit), each behind the auth interceptor. The + `127.0.0.1:9420` behind a keyring bearer token, serving eight services (System, + Accounts, Mail, Filters, Calendar, Contacts, Export, Audit), each behind the + auth interceptor. The `IMAP → store → read → SMTP send` spine works end-to-end over gRPC via the `nuncio-cli` reference client, tested offline. `nuncio.v1` is published with a contract-stability golden. -- **Phase 3 (feature completeness) is now COMPLETE — M1–M4 all merged to `dev`.** +- **The feature surface (Phase 3 through M5) is now COMPLETE — M1–M5 all merged + to `dev`** (M1–M4 already promoted to `main`; M5 promotes next). Filter correctness (3.A), Calendar (M1: CalDAV + RRULE recurrence, API vertical), Contacts (M2: real CardDAV engine + API vertical), JMAP (M3: real client wired - into Mail sync), and Filters made live (M4: engine wired into sync with a + into Mail sync), Filters made live (M4: engine wired into sync with a fire-once guard, the `account`-field bug fixed, edit/export/import/logs and - `Filters.Triage` bulk rescan on the gRPC service) are all done. -- **Next milestone: M5 — Mail lifecycle + incremental sync** (account - edit/delete/test-connection + persisted TLS mode, incremental IMAP sync with - progress + per-item FETCH timeout, explicit `SendMessage` account selection, - keyring rollback on failed `AddAccount`). Then M6 (security hardening), then - M7 (API freeze / client-readiness). + `Filters.Triage` bulk rescan on the gRPC service), and Mail lifecycle + + incremental sync (M5: account edit/delete/test-connection with persisted + `imap_tls_mode`/`smtp_tls_mode` driving the real IMAP/SMTP connection, a real + per-folder `{uidvalidity}:{uidnext}` incremental-sync checkpoint with a safe + full re-fetch on UIDVALIDITY change and a per-item FETCH timeout, explicit + `SendMessage` account selection, and keyring rollback on failed `AddAccount`) + are all done. +- **Next milestone: M6 — Security & release hardening** (fail-closed + encryption, zeroized key material, WORM/ledger key-length validation, + non-loopback gRPC bind rejection + an auth-coverage canary, a fail-closed + updater on missing `SHA256SUMS`, real CIDR/DNS SSRF checks for webhooks, + inbound HTML email sanitization). Then M7 (API freeze / client-readiness). - Full detail and the path to a client-ready API: `docs/ROADMAP.md`. ## Branches & preserved WIP @@ -94,7 +102,7 @@ GitHub milestones **M1–M7** mirror `docs/ROADMAP.md`. Each milestone holds `story`-labelled issues with a full, self-contained spec. Order: M1 → M7 (M1–M5 independent-ish feature work; M6 security; M7 the client-readiness gate). -**M1–M4 are DELIVERED — all stories merged to `dev`.** M5 is next. +**M1–M5 are DELIVERED — all stories merged to `dev`.** M6 is next. **M1 — Finish Calendar (CalDAV)** · milestone #23 · **DONE** - #185 — Add Calendar API vertical (proto, daemon, CLI, E2E) · **[PROTO]** · merged @@ -113,13 +121,13 @@ independent-ish feature work; M6 security; M7 the client-readiness gate). - #192 — Move filter edit/export/import/logs onto the Filters gRPC service · **[PROTO]** · merged - #193 — Re-expose bulk filter triage as a streaming Filters RPC · **[PROTO]** *(depends on #190)* · merged -**M5 — Mail lifecycle + incremental sync** · milestone #27 ← **next** -- #194 — Account lifecycle over gRPC (edit/delete/test) + persist TLS mode · **[PROTO]** *(prior art: `wip/unreviewed-account-tls-sync`, reference-not-trust)* -- #195 — Incremental IMAP sync with progress reporting and per-item FETCH timeout -- #196 — SendMessage selects an explicit account instead of the first configured · **[PROTO]** -- #197 — Roll back orphaned keyring secret when AddAccount fails to persist +**M5 — Mail lifecycle + incremental sync** · milestone #27 · **DONE** +- #194 — Account lifecycle over gRPC (edit/delete/test) + persist TLS mode · **[PROTO]** *(prior art: `wip/unreviewed-account-tls-sync`, reference-not-trust)* · merged +- #195 — Incremental IMAP sync with progress reporting and per-item FETCH timeout · merged +- #196 — SendMessage selects an explicit account instead of the first configured · **[PROTO]** · merged +- #197 — Roll back orphaned keyring secret when AddAccount fails to persist · merged -**M6 — Security & release hardening** · milestone #28 +**M6 — Security & release hardening** · milestone #28 ← **next** - #198 — Fail closed on encryption failure in `PayloadCipher::encrypt_text_at_rest` - #199 — Zeroize long-lived engine key material on drop - #200 — Validate WORM/ledger key length from the vault, fail closed on mismatch @@ -145,7 +153,7 @@ independent-ish feature work; M6 security; M7 the client-readiness gate). those two issues if you consider that sensitive. - Decide the eventual fate of `wip/unreviewed-account-tls-sync` (review-and-adopt vs discard) once M5 lands a clean implementation. -- Follow-ups filed during M1–M4 that are open but not blocking (not scheduled +- Follow-ups filed during M1–M5 that are open but not blocking (not scheduled into a milestone yet): #216 (calendar windowing/predicate refinement), #219 (contacts notes handling), #225 (`filter edit` silently re-enables a disabled - rule and resets `created_at`). + rule and resets `created_at`), #233 (M5 sync/SMTP robustness polish). diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 1eed593..e3eac3d 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -53,8 +53,9 @@ offline. The legacy JSON-RPC IPC is removed. The `nuncio.v1` contract is published with a byte-deterministic `FileDescriptorSet` golden guarding it, plus external-codegen docs for Swift/C#/TypeScript. -**Phase 3 — Feature completeness — is now COMPLETE**, via milestones M1–M4, all -merged to `dev`: +**The feature surface — Phase 3 through M5 — is now COMPLETE**, via milestones +M1–M5, all merged to `dev` (M1–M4 are already promoted to `main`; M5 promotes +next): - **3.A Filter correctness** — **done** (account scoping, `ON ACCOUNT`, non-ASCII offsets, `NOT IN`). @@ -72,18 +73,30 @@ merged to `dev`: honestly rejected, filter edit/export/import/logs moved onto the `Filters` gRPC service, and a `Filters.Triage` server-streaming RPC does retroactive bulk rescan. +- **Mail lifecycle & incremental sync** — **done** (M5). Account lifecycle + (`UpdateAccount`/`RemoveAccount`/`TestAccountConnection`) is real over gRPC, + with `imap_tls_mode`/`smtp_tls_mode` persisted and actually driving the real + IMAP (implicit/STARTTLS/plain) and SMTP connection. Sync is genuinely + incremental: a real per-folder `{uidvalidity}:{uidnext}` checkpoint narrows + the FETCH instead of re-fetching everything, a UIDVALIDITY change forces a + safe full re-fetch (no silent mail loss), a per-item FETCH timeout replaces + the indefinite hang, and a `SyncProgress` event is emitted. `SendMessage` + takes an explicit `account_id` (honest `AccountNotFound`, no silent + wrong-account fallback), and `AddAccount` rolls back an orphaned keyring + secret if persistence fails. -**Next up: M5 — Mail lifecycle & incremental sync** (account edit/delete/test + -persisted TLS mode, incremental IMAP sync with progress + per-item FETCH -timeout, explicit `SendMessage` account selection, keyring rollback on failed -`AddAccount`). Then M6 (security hardening), then M7 (API freeze / client -readiness). +**Next up: M6 — Security & release hardening** (fail-closed encryption, +zeroized key material, WORM/ledger key-length validation, a rejection of +non-loopback gRPC binds plus an auth-coverage canary, a fail-closed updater on +missing `SHA256SUMS`, real CIDR/DNS SSRF checks for webhooks, inbound HTML +email sanitization). Then M7 (API freeze / client-readiness). -A handful of follow-ups were filed during M1–M4 and remain open, tracked as -their own backlog items rather than blocking M1–M4's completion: calendar -windowing/predicate refinement (#216), contacts notes handling (#219), and a +A handful of follow-ups were filed during M1–M5 and remain open, tracked as +their own backlog items rather than blocking completion: calendar +windowing/predicate refinement (#216), contacts notes handling (#219), a `filter edit` bug where re-saving a rule silently re-enables it and resets -`created_at` (#225). +`created_at` (#225), and further mail sync/SMTP robustness polish identified +during M5 (#233). **Explicitly deferred** (out of scope until the backend is client-ready and each is re-justified as a genuine engine capability): OpenPGP/S-MIME E2EE (#79), diff --git a/docs/roadmap/index.html b/docs/roadmap/index.html index c09c6fb..524df65 100644 --- a/docs/roadmap/index.html +++ b/docs/roadmap/index.html @@ -76,7 +76,7 @@ /* milestone rail */ .miles{position:relative;margin-top:8px} .miles::before{content:"";position:absolute;left:19px;top:12px;bottom:34px;width:2px; - background:linear-gradient(var(--done),var(--done) 57%,var(--prog) 57%,var(--prog) 71%,var(--line) 71%);border-radius:2px} + background:linear-gradient(var(--done),var(--done) 71%,var(--prog) 71%,var(--prog) 86%,var(--line) 86%);border-radius:2px} @media (max-width:640px){.miles::before{display:none}} .m{position:relative;padding-left:52px;margin-bottom:16px} @media (max-width:640px){.m{padding-left:0}} @@ -135,14 +135,15 @@

Where we are · 2026-07-30

-

Phases 0–2 complete, Phase 3 feature completeness done — M1–M4 all merged

+

Phases 0–2 complete, feature surface (Phase 3–M5) done — M1–M5 all merged

The daemon boots gRPC-only on loopback 127.0.0.1:9420 behind a keyring-minted bearer token. IMAP → store → read → SMTP send runs end-to-end over gRPC, driven by the nuncio-cli reference client, tested fully offline. The legacy JSON-RPC IPC is gone, and nuncio.v1 is published with a byte-deterministic contract-stability golden. - Milestones M1–M4 are now complete on dev: Calendar, Contacts, JMAP, and live - Filters are all real, wired, and API-complete. Next up: M5 (mail lifecycle - & incremental sync).

+ Milestones M1–M5 are now complete on dev (M1–M4 already promoted to + main): Calendar, Contacts, JMAP, live Filters, and mail lifecycle + incremental + sync are all real, wired, and API-complete. Next up: M6 (security & + release hardening).

SystemAccountsMail FiltersCalendarContacts @@ -158,6 +159,7 @@ M2 · Contacts (CardDAV) M3 · JMAP M4 · Filters made live + M5 · Mail lifecycle + incremental sync
@@ -226,27 +228,31 @@ -
+
M5

M5Mail lifecycle + incremental sync

- Next + Done
-

Round out real account management: edit / delete / test-connection with TLS modes - (implicit / STARTTLS / plain) actually wired; explicit send-account selection; and incremental sync - (honor since_state instead of re-fetching the whole mailbox every time) with progress and stall timeouts.

+

Account lifecycle (edit / delete / test-connection) is real over gRPC, with TLS modes + (implicit / STARTTLS / plain) persisted and actually driving the IMAP/SMTP connection. Sync is now + genuinely incremental via a per-folder {uidvalidity}:{uidnext} checkpoint (a UIDVALIDITY + change forces a safe full re-fetch — no silent mail loss), with a per-item FETCH timeout and + SyncProgress events. SendMessage takes an explicit account id (honest + AccountNotFound, no silent wrong-account fallback), and a failed AddAccount + rolls back its orphaned keyring secret.

Exit: full account lifecycle + incremental sync, offline-E2E-proven, no fabricated IMAP fallbacks.
-
#170#169#182#183#184
+
#170#169#182#183#184done — #194–#197 merged
-
+
M6

M6Security & release hardening

- Planned + Next

Fail-closed everywhere: fix the silent empty-ciphertext data-loss path; a signature-verifying updater that refuses to apply without checksums; zeroize key material; validate vault -- 2.54.0