From ce4951589285e7e5d2dd4ec9e2522368e72bf4ef Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 14 Aug 2026 20:22:53 -0500 Subject: [PATCH 1/2] fix(store): reject unreadable stored TLS modes on account read tls_mode_from_db coerced any unrecognized imap_tls_mode/smtp_tls_mode token to ImplicitTls on the path every ordinary account read takes, so a corrupted or tampered row silently handed callers a transport the operator never configured -- a STARTTLS account read back as implicit TLS, with no error and no log. Parse now fails closed with DatabaseError::Corrupted naming the account, the column, and the offending value. list_accounts already returned a Result, so no caller signature changes and no caller panics; get_account inherits the behaviour through it. Refs #271 --- crates/nuncio-store/src/db.rs | 122 ++++++++++++++++++++++++++++++---- 1 file changed, 108 insertions(+), 14 deletions(-) diff --git a/crates/nuncio-store/src/db.rs b/crates/nuncio-store/src/db.rs index 12ef57e..ca96d34 100644 --- a/crates/nuncio-store/src/db.rs +++ b/crates/nuncio-store/src/db.rs @@ -413,13 +413,36 @@ fn tls_mode_to_db(mode: nuncio_core::TlsMode) -> &'static str { } /// 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 { +/// [`nuncio_core::TlsMode`]. +/// +/// Fails closed. TLS modes are validated before they are written, so the only +/// way an unrecognized token reaches this function is corruption or tampering +/// of the row. Coercing such a value to a default would silently hand the +/// caller a transport the operator never configured -- reading `"start_tls"` +/// as implicit TLS, or an unreadable value as *any* concrete mode, changes the +/// account's security posture with no signal at all. The account's real mode is +/// unknown at that point, and an unknown security posture is not something this +/// layer may guess at, so the read is refused and the offending account, column, +/// and raw value are named in the error. +/// +/// This is deliberately stricter than the best-effort recovery path in +/// [`crate::recovery`]: salvage exists to rescue what it can from an already +/// broken file and has no caller to return an error to, whereas this is the +/// path every ordinary account read takes. +fn tls_mode_from_db( + account_id: &str, + column: &str, + value: &str, +) -> Result { match value { - "start_tls" => nuncio_core::TlsMode::StartTls, - "plain" => nuncio_core::TlsMode::Plain, - _ => nuncio_core::TlsMode::ImplicitTls, + "implicit_tls" => Ok(nuncio_core::TlsMode::ImplicitTls), + "start_tls" => Ok(nuncio_core::TlsMode::StartTls), + "plain" => Ok(nuncio_core::TlsMode::Plain), + other => Err(DatabaseError::Corrupted(format!( + "account {account_id} has an unrecognized {column} value {other:?}; refusing to \ + load it under a guessed TLS mode -- re-save the account with a valid transport \ + security setting" + ))), } } @@ -1369,8 +1392,7 @@ impl DatabaseEngine { .await .map_err(DatabaseError::Query)?; - Ok(rows - .into_iter() + rows.into_iter() .map( |( id, @@ -1387,7 +1409,8 @@ impl DatabaseEngine { smtp_tls_mode, collection_url, filters_enabled, - )| { + )| + -> Result { let protocol = serde_json::from_str(&protocol_str) .unwrap_or(nuncio_core::AccountProtocol::ImapSmtp); // Reconstruct the exact transport variant from the @@ -1410,10 +1433,18 @@ impl DatabaseEngine { nuncio_core::Transport::ImapSmtp(nuncio_core::ImapSmtpTransport { imap_host: server_host, imap_port: server_port as u16, - imap_tls_mode: tls_mode_from_db(&imap_tls_mode), + imap_tls_mode: tls_mode_from_db( + &id, + "imap_tls_mode", + &imap_tls_mode, + )?, smtp_host: resolved_smtp_host, smtp_port: resolved_smtp_port, - smtp_tls_mode: tls_mode_from_db(&smtp_tls_mode), + smtp_tls_mode: tls_mode_from_db( + &id, + "smtp_tls_mode", + &smtp_tls_mode, + )?, }) } nuncio_core::AccountProtocol::Jmap => { @@ -1427,7 +1458,7 @@ impl DatabaseEngine { }) } }; - nuncio_core::AccountConfig { + Ok(nuncio_core::AccountConfig { id, name, email_address, @@ -1435,10 +1466,10 @@ impl DatabaseEngine { sync_interval_secs: sync_interval_secs as u64, filters_enabled: filters_enabled != 0, transport, - } + }) }, ) - .collect()) + .collect() } /// Fetch the persisted inbound-sync checkpoint for a folder, or `None` if @@ -4289,6 +4320,69 @@ mod tests { assert_eq!(fetched, acct); } + /// An unreadable stored TLS mode must fail the read rather than resolve to + /// a guessed one. The account below is configured for STARTTLS; its stored + /// mode is then overwritten out-of-band with a token this build does not + /// know. Previously that token was coerced to implicit TLS, so every + /// ordinary account read handed the caller a transport the operator never + /// chose, with nothing anywhere to say so. Both the IMAP and the SMTP + /// column are checked, and `get_account` is checked alongside + /// `list_accounts` because it reads through the same path. + #[tokio::test] + async fn unrecognized_stored_tls_mode_fails_the_read_instead_of_defaulting() { + for column in ["imap_tls_mode", "smtp_tls_mode"] { + let (engine, _dir) = DatabaseEngine::connect_ephemeral().await.unwrap(); + + let acct = nuncio_core::AccountConfig { + id: "acct-tls-corrupt".to_string(), + name: "Tampered TLS Account".to_string(), + email_address: "tampered@nuncio.mx".to_string(), + keyring_secret_key: "nuncio/acct-tls-corrupt".to_string(), + sync_interval_secs: 60, + filters_enabled: false, + transport: nuncio_core::Transport::ImapSmtp(nuncio_core::ImapSmtpTransport { + imap_host: "imap.nuncio.mx".to_string(), + imap_port: 143, + imap_tls_mode: nuncio_core::TlsMode::StartTls, + smtp_host: "smtp.nuncio.mx".to_string(), + smtp_port: 587, + smtp_tls_mode: nuncio_core::TlsMode::StartTls, + }), + }; + engine.save_account(&acct).await.expect("save succeeds"); + + // Simulates corruption or tampering of the row: writes are + // validated, so this value could never arrive through the API. + sqlx::query(&format!("UPDATE accounts SET {column} = ? WHERE id = ?")) + .bind("tls") + .bind("acct-tls-corrupt") + .execute(&engine.pool) + .await + .expect("corrupting update succeeds"); + + let err = engine + .list_accounts() + .await + .expect_err("an unreadable TLS mode must not resolve to a default"); + let message = err.to_string(); + assert!( + matches!(err, DatabaseError::Corrupted(_)), + "expected a corruption error, got: {message}" + ); + for expected in ["acct-tls-corrupt", column, "tls"] { + assert!( + message.contains(expected), + "error must name {expected}, got: {message}" + ); + } + + engine + .get_account("acct-tls-corrupt") + .await + .expect_err("get_account must fail closed on the same row"); + } + } + /// A CalDAV account's `collection_url` must survive save/reload, and a /// mail account's empty `collection_url` must round-trip as empty (not /// NULL-mangled), proving the backfill-safe column persists both. -- 2.54.0 From 98b59b8a981e6cad744f342acc82b49cf520ffc2 Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 14 Aug 2026 20:22:58 -0500 Subject: [PATCH 2/2] fix(filter): size counts all body parts and attachments The NSQL size field was computed from body_plain alone, so an HTML-only message measured zero bytes and a short note carrying a large attachment measured only the note. Every size comparison then answered confidently and wrongly on exactly the mail such rules are written to catch. size is now the total byte size of the content the store holds: both body parts plus every attachment payload. Both parts are summed because a multipart/alternative message genuinely carries both on the wire. Headers and MIME framing are excluded and documented as excluded -- the raw RFC822 octets are not retained and no column carries their length, so the figure is a consistent lower bound rather than a guess. Refs #256 --- crates/nuncio-filter/src/ast.rs | 3 +- crates/nuncio-filter/src/engine.rs | 128 +++++++++++++++++++++++++++-- 2 files changed, 125 insertions(+), 6 deletions(-) diff --git a/crates/nuncio-filter/src/ast.rs b/crates/nuncio-filter/src/ast.rs index 2719e59..a184982 100644 --- a/crates/nuncio-filter/src/ast.rs +++ b/crates/nuncio-filter/src/ast.rs @@ -17,7 +17,8 @@ pub enum FilterField { Header(String), /// Attachment presence indicator (`true`/`false`). HasAttachment, - /// Message size in bytes. + /// Message size in bytes: all stored body parts plus all attachment + /// payloads. Excludes headers and MIME framing, which are not retained. Size, /// Current folder location (e.g. `INBOX`). Folder, diff --git a/crates/nuncio-filter/src/engine.rs b/crates/nuncio-filter/src/engine.rs index 9139b6c..34d861b 100644 --- a/crates/nuncio-filter/src/engine.rs +++ b/crates/nuncio-filter/src/engine.rs @@ -97,6 +97,42 @@ impl CompiledFilter { } } + /// Total byte size of the message content Nuncio holds: every body part + /// plus every attachment payload. + /// + /// This is what `WHERE size > N` compares against. Counting only the + /// plaintext body would report `0` for the very common HTML-only message + /// and would ignore attachments entirely, so a size rule would quietly + /// return the wrong verdict on exactly the mail it is usually written to + /// catch (the big ones). Both body parts are summed rather than one being + /// preferred: a `multipart/alternative` message genuinely carries the + /// plaintext *and* the HTML on the wire, so summing them tracks the real + /// message rather than one arbitrary view of it. + /// + /// The figure is the size of the decoded content as stored, not the exact + /// RFC822 octet count of the original transmission -- headers, MIME + /// boundaries, and transfer encoding are not counted, because the raw + /// octets are not retained and no stored column carries their length. It is + /// therefore a consistent lower bound on the wire size, never a guess: for + /// base64-encoded attachments the transmitted form is roughly a third + /// larger again. Rules are written against what the store actually has. + fn message_size_bytes(email: &Email) -> i64 { + let mut total: u64 = 0; + for part in [email.body_plain.as_deref(), email.body_html.as_deref()] + .into_iter() + .flatten() + { + total = total.saturating_add(part.len() as u64); + } + for attachment in &email.attachments { + total = total.saturating_add(attachment.content.len() as u64); + } + // Saturate rather than wrap: a size past `i64::MAX` is not a real + // message, and a wrapped negative would make `size > N` answer the + // opposite of the truth. + i64::try_from(total).unwrap_or(i64::MAX) + } + fn eval_leaf( leaf: &ConditionLeaf, placed: PlacedEmail<'_>, @@ -146,11 +182,7 @@ impl CompiledFilter { } } FilterField::Size => { - let size = email - .body_plain - .as_ref() - .map(|b| b.len() as i64) - .unwrap_or(0); + let size = Self::message_size_bytes(email); if let FilterValue::Number(target) = leaf.value { match leaf.operator { FilterOperator::Equals => size == target, @@ -526,6 +558,92 @@ mod tests { assert_eq!(preview.matched_rule_name, Some("Urgent".to_string())); } + /// Evaluate one NSQL rule against one message, returning whether it fired. + fn size_rule_matches(nsql: &str, email: &Email) -> bool { + let rule = crate::parser::NsqlParser::parse_rule("Size Rule", 1, nsql).unwrap(); + let engine = FilterEngine::new(vec![rule]).unwrap(); + let (_, placement) = test_placed_email(&email.account_id, "", "inbox"); + !engine + .evaluate(PlacedEmail { + email, + placement: &placement, + }) + .is_empty() + } + + /// `size` must reflect every body part and every attachment, not the + /// plaintext body alone. An HTML-only message has no plaintext body at + /// all, so it used to measure zero bytes and silently fell on the wrong + /// side of every size comparison -- `size > 100` said no for a 400-byte + /// message, and `size < 100` said yes. + #[test] + fn size_counts_html_body_of_html_only_message() { + let (mut email, _) = test_placed_email("acct-1", "HTML only", "inbox"); + email.body_plain = None; + email.body_html = Some(format!("

{}

", "x".repeat(400))); + + assert!( + size_rule_matches("WHERE size > 100 ACTION MARK READ", &email), + "a 407-byte HTML-only message must satisfy size > 100" + ); + assert!( + !size_rule_matches("WHERE size < 100 ACTION MARK READ", &email), + "a 407-byte HTML-only message must not satisfy size < 100" + ); + } + + /// Attachment payloads are part of the message's size. A short note + /// carrying a large attachment is exactly the mail a size rule is written + /// to catch, and counting only the note's text missed it entirely. + #[test] + fn size_counts_attachment_payloads() { + let (mut email, _) = test_placed_email("acct-1", "Invoice", "inbox"); + email.body_plain = Some("See attached.".to_string()); + email.attachments = vec![nuncio_core::model::Attachment { + filename: "invoice.pdf".to_string(), + mime_type: "application/pdf".to_string(), + content: vec![0u8; 5_000].into(), + }]; + + assert!( + size_rule_matches("WHERE size > 4096 ACTION MARK READ", &email), + "a 13-byte note with a 5000-byte attachment must satisfy size > 4096" + ); + } + + /// A `multipart/alternative` message carries both renderings on the wire, + /// so both are counted rather than one being preferred -- this pins the + /// definition `size` is documented to have. + #[test] + fn size_sums_both_body_parts_and_attachments() { + let (mut email, _) = test_placed_email("acct-1", "Both parts", "inbox"); + email.body_plain = Some("a".repeat(100)); + email.body_html = Some("b".repeat(250)); + email.attachments = vec![nuncio_core::model::Attachment { + filename: "logo.png".to_string(), + mime_type: "image/png".to_string(), + content: vec![0u8; 650].into(), + }]; + + assert_eq!(CompiledFilter::message_size_bytes(&email), 1_000); + assert!(size_rule_matches( + "WHERE size = 1000 ACTION MARK READ", + &email + )); + } + + /// A message with neither body part nor attachments genuinely measures + /// zero, and must still compare as a number rather than being skipped. + #[test] + fn size_of_empty_message_is_zero() { + let (mut email, _) = test_placed_email("acct-1", "Empty", "inbox"); + email.body_plain = None; + email.body_html = None; + + assert_eq!(CompiledFilter::message_size_bytes(&email), 0); + assert!(size_rule_matches("WHERE size < 1 ACTION MARK READ", &email)); + } + #[test] fn test_webhook_signature() { let sig = -- 2.54.0