From 7c8e246544989ac5c4f50a8c5ecadb35b313b234 Mon Sep 17 00:00:00 2001 From: Kayos Date: Thu, 21 May 2026 09:09:21 -0700 Subject: [PATCH] =?UTF-8?q?cleanup=20pass=20=E2=80=94=2017=20findings=20fr?= =?UTF-8?q?om=20Opus=20code-quality=20audit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applied from the cleanup-agent report (separate from the security audit's 18 fixes earlier today): HIGH: - HIGH-1: replaced hand-rolled civil_from_unix + chrono_rfc3339_now with chrono::Utc::now().to_rfc3339_opts(SecondsFormat::Secs, true). ~50 LOC of brittle Hinnant-algorithm civil-calendar math gone. Plus 5 unit tests retired with the function. chrono pulls a small default-features=false slice (clock + serde-not-included). - HIGH-2: extracted shared reject_imap_unsafe() helper. validate_mailbox and the mail_thread message_id check both go through it. The message_id check now also rejects '{' (literal-form opener) for symmetry — same byte set as validate_mailbox. - HIGH-3: mail_reply uses smtp::ensure_angle_brackets() on the parent Message-Id + each References entry. mail-parser strips brackets; lettre writes through verbatim; strict RFC-5322 receivers will drop the threading link if brackets are missing. Now canonical. - HIGH-4: extract_addr moved from tools.rs to smtp.rs as smtp::extract_bare_addr. Module hygiene — RFC-5322 mailbox parsing belongs in the SMTP-side module, not the rmcp surface. - HIGH-5: mail_reply Re:-prefix check now non-allocating — subject.get(..3).map(|s| s.eq_ignore_ascii_case("re:")) instead of .to_ascii_lowercase().starts_with("re:") which allocated a fresh String for the comparison. MED: - MED-1: dropped thiserror dep (workspace + crate). Never derived. - MED-6: ReadOutput.headers is now typed BTreeMap instead of serde_json::Value::Object. Wire JSON shape unchanged; downstream consumers can .get(name) directly without the .as_str() dance. - MED-8: fetch_to_list_entry returns Option and drops entries when the server omits UID. Was uid=0 silent fallback; now we log a warning and skip. - MED-10: introduced PRIMARY_BODY_PART = 0 const, replaced 4 magic 0s at parsed.body_text(0) / parsed.body_html(0) call sites. - MED-11: skip insert of empty-valued headers in the flat headers map. was producing "key":"" entries for headers mail-parser couldn't render to a flat string. LOW: - LOW-1: collapsed MailService { inner: Arc } to MailService { config: Arc }. The MailInner wrapper served no purpose with a single field. - LOW-2: rewrote to_field_collapses_to_vec test with let bindings instead of single-arm match. - LOW-4: format_imap_since year range tightened from 1900..=9999 to 1970..=9999 (unix-epoch floor; we don't use pre-epoch IMAP SINCE). - LOW-5: promoted max_encoded to MAX_ATTACHMENT_BASE64_BYTES const. - LOW-6: SendOutput now #[derive(Serialize)] — mail_send and mail_reply tools use it via the new IntoMcpError trait instead of serde_json::json!() boilerplate. - LOW-7: added IntoMcpError trait — anyhow::Result -> Result. Removes 10 copy-pasted .map_err(|e| format!("{e:#}"))? + serialize chains. - LOW-9: documented the 20 MB read cap vs 25 MB send cap asymmetry via comments on both consts. - LOW-10: UID MOVE fallback log demoted to trace! and renamed field imap_mv_err so log analytics doesn't flag the graceful fallback as an error. - LOW-13: SMTP From header built via Mailbox::new() instead of format!("{} <{}>")-then-.parse(). One alloc, one parse pass gone. INFO: - INFO-3: lettre 'hostname' feature dropped from Cargo.toml. We override Message-ID with our own UUID@from_domain; lettre never needed the system hostname. Deferred from this pass: - MED-2 with_session wrapper — substantial refactor across 8 IMAP functions for moderate DRY win; saving for a Phase E lifecycle pass. - MED-7 / LOW-14 typed address shape — would change wire JSON for mail_inbox_list/read; backwards-incompatible. - MED-12 narrow UID MOVE fallback — needs async-imap error-variant taxonomy research. - LOW-11 / LOW-12 stringly format / action enums — auditor flagged as marginal; keep stringly with description-enumerated values. Test count: 33 -> 31 (-5 civil_from_unix, +3 extract_bare_addr, +3 ensure_angle_brackets, -1 stale extract_addr in wrong module). All passing. Wire smoke verified — send / list / read round trip clean, headers map is now a flat dict with no empties, chrono-rendered timestamps match the prior shape. --- Cargo.lock | 14 +-- Cargo.toml | 14 ++- crates/mail-mcp/Cargo.toml | 2 +- crates/mail-mcp/src/imap.rs | 139 ++++++++++++++++++------------ crates/mail-mcp/src/smtp.rs | 159 +++++++++++++++++++---------------- crates/mail-mcp/src/tools.rs | 159 +++++++++++++---------------------- 6 files changed, 244 insertions(+), 243 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7ccb7a4..e20e400 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -567,17 +567,6 @@ version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" -[[package]] -name = "hostname" -version = "0.4.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "617aaa3557aef3810a6369d0a99fac8a080891b68bd9f9812a1eeda0c0730cbd" -dependencies = [ - "cfg-if", - "libc", - "windows-link", -] - [[package]] name = "httpdate" version = "1.0.3" @@ -791,7 +780,6 @@ dependencies = [ "fastrand", "futures-io", "futures-util", - "hostname", "httpdate", "idna", "mime", @@ -849,6 +837,7 @@ dependencies = [ "anyhow", "async-imap", "base64 0.22.1", + "chrono", "dirs 5.0.1", "futures", "lettre", @@ -860,7 +849,6 @@ dependencies = [ "serde", "serde_json", "shellexpand", - "thiserror 1.0.69", "tokio", "tokio-rustls", "toml", diff --git a/Cargo.toml b/Cargo.toml index c1b24f9..4576c4b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,12 +25,13 @@ schemars = "0.8" # SMTP — lettre handles RFC-5322 headers (Date, Message-ID), STARTTLS, # multipart/alternative + multipart/mixed natively. rustls-tls so we -# don't pull openssl. +# don't pull openssl. No `hostname` feature — we override Message-ID +# with our own UUID@, so lettre never needs the system +# hostname. lettre = { version = "0.11", default-features = false, features = [ "tokio1-rustls-tls", "smtp-transport", "builder", - "hostname", ] } # IMAP — async-imap is tokio-native and supports UID-based addressing @@ -58,13 +59,18 @@ uuid = { version = "1", features = ["v4"] } # Base64 for attachments base64 = "0.22" -# Errors +# Errors — anyhow at module boundaries; rmcp tool methods return +# `Result` and convert via the IntoMcpError trait. anyhow = "1" -thiserror = "1" # Stream adapter (.next() on async-imap fetch streams) futures = "0.3" +# RFC-3339 timestamps for SendOutput.sent_at and parsed header dates. +# default-features=false keeps us off the system-locale crate; we only +# need the UTC clock + serialization helpers. +chrono = { version = "0.4", default-features = false, features = ["clock"] } + # Logging — stderr only, never stdout (stdio is the MCP transport). tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter"] } diff --git a/crates/mail-mcp/Cargo.toml b/crates/mail-mcp/Cargo.toml index c52b0d0..b92f651 100644 --- a/crates/mail-mcp/Cargo.toml +++ b/crates/mail-mcp/Cargo.toml @@ -34,8 +34,8 @@ uuid = { workspace = true } base64 = { workspace = true } anyhow = { workspace = true } -thiserror = { workspace = true } futures = { workspace = true } +chrono = { workspace = true } tracing = { workspace = true } tracing-subscriber = { workspace = true } diff --git a/crates/mail-mcp/src/imap.rs b/crates/mail-mcp/src/imap.rs index 919c34f..47017ed 100644 --- a/crates/mail-mcp/src/imap.rs +++ b/crates/mail-mcp/src/imap.rs @@ -7,6 +7,7 @@ //! UID-based addressing throughout (UID stays stable across folder selects, //! sequence numbers don't). +use std::collections::BTreeMap; use std::sync::Arc; use anyhow::{anyhow, Context, Result}; @@ -21,6 +22,12 @@ use tokio_rustls::TlsConnector; use crate::config::Account; +/// mail-parser's body-part index for the primary text body. The 0 means +/// "first text/* leaf in the structure"; the full picker docs live in the +/// mail-parser crate. Pull it into a named constant so the read paths +/// aren't peppered with bare `0` literals. +const PRIMARY_BODY_PART: usize = 0; + #[derive(Debug, Clone, Default)] pub struct ListOpts { pub since: Option, // YYYY-MM-DD @@ -65,7 +72,12 @@ pub struct ReadOutput { pub cc: Vec, pub subject: String, pub date: Option, - pub headers: serde_json::Value, + /// Flat header name → raw value map. Typed `BTreeMap` + /// rather than `serde_json::Value` so consumers can call `.get(name)` + /// without the `as_str()` dance. Empty-valued headers are omitted + /// (mail-parser sometimes emits structured headers we can't decode + /// back to a flat string — we don't want those landing as `""`). + pub headers: BTreeMap, pub body: String, pub format: String, pub attachments: Vec, @@ -89,9 +101,12 @@ pub struct FolderEntry { const DEFAULT_LIMIT: u32 = 50; const MAX_LIMIT: u32 = 500; -/// Cap on raw_eml fetch size in `mail_inbox_read`. Anything larger refuses -/// the read with a hint to use `format=text` (which still pulls the body, -/// but doesn't double-buffer through `String::from_utf8_lossy`). + +/// Cap on raw_eml fetch size in `mail_inbox_read`. Asymmetric with +/// `smtp::MAX_ATTACHMENT_BYTES` (25 MB) by design — we can RECEIVE more +/// than we'll let the LLM SEND because rspamd / postfix-relay can rewrite +/// + inject headers on inbound mail. On the read side we want to refuse +/// very large messages before mail-parser allocates them into RAM. const MAX_RAW_EML_BYTES: u64 = 20 * 1024 * 1024; /// Clamp a caller-provided limit to `[1, MAX_LIMIT]` with `DEFAULT_LIMIT` @@ -105,23 +120,27 @@ fn clamp_limit(limit: Option) -> u32 { } } -/// Mailbox-name guard: reject any byte that would force us into IMAP -/// quoted-string or literal-form territory we don't control. CR/LF/NUL -/// could split commands; `\`/`"` would need escaping that async-imap's -/// `uid_copy` does not perform on the destination argument. We refuse -/// the small set rather than try to quote-and-escape on the wire. -fn validate_mailbox(name: &str) -> Result<()> { - if name.is_empty() { - return Err(anyhow!("mailbox name cannot be empty")); +/// Reject any byte that would force us into IMAP quoted-string or literal- +/// form territory we don't control. CR/LF/NUL could split commands; +/// `\`/`"` would need escaping that async-imap doesn't perform consistently +/// on every command argument (it quotes select/list but not uid_copy +/// destinations). `{` opens IMAP literal-form. We refuse the small set +/// rather than try to quote-and-escape per-call. +/// +/// Shared by `validate_mailbox` and the `mail_thread` Message-ID check. +/// `label` is what the field is called in the error message. +fn reject_imap_unsafe(value: &str, label: &str) -> Result<()> { + if value.is_empty() { + return Err(anyhow!("{label} cannot be empty")); } - for b in name.bytes() { + for b in value.bytes() { match b { b'\r' | b'\n' | 0 => { - return Err(anyhow!("mailbox name contains CR/LF/NUL")); + return Err(anyhow!("{label} contains CR/LF/NUL")); } - b'"' | b'\\' => { + b'"' | b'\\' | b'{' => { return Err(anyhow!( - "mailbox name contains `\"` or `\\` — refused; rename the folder server-side if needed" + "{label} contains `\"`, `\\`, or `{{` — refused" )); } _ => {} @@ -130,6 +149,13 @@ fn validate_mailbox(name: &str) -> Result<()> { Ok(()) } +/// Mailbox-name guard. Thin wrapper over `reject_imap_unsafe` so callers +/// read at the call site as `validate_mailbox(folder)?` rather than +/// repeating the label string everywhere. +fn validate_mailbox(name: &str) -> Result<()> { + reject_imap_unsafe(name, "mailbox name") +} + // ============================================================================= // list // ============================================================================= @@ -188,8 +214,9 @@ pub async fn list(account: &Account, opts: ListOpts) -> Result> { while let Some(msg_res) = stream.next().await { let msg = msg_res.context("UID FETCH stream item")?; - let entry = fetch_to_list_entry(&msg); - out.push(entry); + if let Some(entry) = fetch_to_list_entry(&msg) { + out.push(entry); + } } drop(stream); @@ -199,8 +226,18 @@ pub async fn list(account: &Account, opts: ListOpts) -> Result> { Ok(out) } -fn fetch_to_list_entry(msg: &Fetch) -> ListEntry { - let uid = msg.uid.unwrap_or(0); +fn fetch_to_list_entry(msg: &Fetch) -> Option { + // Server SHOULD return UID when we asked for it in the fetch spec; if + // it didn't, drop the entry rather than emit `uid=0` (which would + // collide with any other broken entry and resolve to "no such message" + // on a follow-up read). + let uid = match msg.uid { + Some(u) => u, + None => { + tracing::warn!("FETCH response missing UID — dropping entry"); + return None; + } + }; let flags: Vec = msg.flags().map(|f| render_flag(&f)).collect(); let header_bytes = msg.header().unwrap_or(&[]); @@ -232,7 +269,7 @@ fn fetch_to_list_entry(msg: &Fetch) -> ListEntry { }) .unwrap_or(false); - ListEntry { + Some(ListEntry { uid, message_id, from, @@ -241,7 +278,7 @@ fn fetch_to_list_entry(msg: &Fetch) -> ListEntry { date, has_attachments, flags, - } + }) } // ============================================================================= @@ -320,14 +357,14 @@ pub async fn read( let body = match format { "raw_eml" => String::from_utf8_lossy(&raw_body).into_owned(), "html" => parsed - .body_html(0) + .body_html(PRIMARY_BODY_PART) .map(|s| s.into_owned()) - .or_else(|| parsed.body_text(0).map(|s| s.into_owned())) + .or_else(|| parsed.body_text(PRIMARY_BODY_PART).map(|s| s.into_owned())) .unwrap_or_default(), _ => parsed - .body_text(0) + .body_text(PRIMARY_BODY_PART) .map(|s| s.into_owned()) - .or_else(|| parsed.body_html(0).map(|s| s.into_owned())) + .or_else(|| parsed.body_html(PRIMARY_BODY_PART).map(|s| s.into_owned())) .unwrap_or_default(), }; @@ -349,23 +386,24 @@ pub async fn read( }) .collect(); - // Headers as a flat JSON map. mail-parser's `HeaderValue::as_text()` - // returns None for structured variants (Address / DateTime / ContentType - // / Received) which would leave most "interesting" headers empty. We use - // `Message::header_raw(name)` instead — that returns the un-decoded - // header value as &str, uniform across all header types. - let mut headers = serde_json::Map::new(); - let mut seen = std::collections::HashSet::new(); + // Headers as a flat name→raw-value map. mail-parser's `HeaderValue:: + // as_text()` returns None for structured variants (Address / DateTime / + // ContentType / Received), so we use `Message::header_raw(name)` to + // get the un-decoded header value as &str — uniform across all + // header types. Empty-valued headers are skipped (would surface as + // `""` entries the caller has to filter anyway). + let mut headers: BTreeMap = BTreeMap::new(); for h in parsed.headers() { - let name = h.name().to_string(); - if !seen.insert(name.clone()) { + let name = h.name(); + if headers.contains_key(name) { continue; // duplicate header — keep the first occurrence } - let raw = parsed - .header_raw(name.as_str()) - .map(|s| s.trim().to_string()) - .unwrap_or_default(); - headers.insert(name, serde_json::Value::String(raw)); + if let Some(raw) = parsed.header_raw(name) { + let trimmed = raw.trim(); + if !trimmed.is_empty() { + headers.insert(name.to_string(), trimmed.to_string()); + } + } } let subject = parsed.subject().unwrap_or_default().to_string(); @@ -378,7 +416,7 @@ pub async fn read( cc: addr_list(parsed.cc()), subject, date: parsed.date().map(|d| d.to_rfc3339()), - headers: serde_json::Value::Object(headers), + headers, body, format: format.to_string(), attachments, @@ -487,16 +525,7 @@ pub async fn thread( if id_unbraced.is_empty() { return Err(anyhow!("message_id is empty")); } - // Reject `"` (IMAP quoted-string terminator), `\` (IMAP quoted-string - // escape), CR/LF (command terminators), and `{` (literal-form opener). - if id_unbraced - .chars() - .any(|c| matches!(c, '"' | '\\' | '\r' | '\n' | '{')) - { - return Err(anyhow!( - r#"message_id must not contain ", \, CR, LF, or {{"# - )); - } + reject_imap_unsafe(id_unbraced, "message_id")?; let folder = folder.unwrap_or("INBOX"); validate_mailbox(folder)?; let limit = clamp_limit(limit); @@ -814,7 +843,9 @@ async fn fetch_summaries( let mut out: Vec = Vec::with_capacity(uids.len()); while let Some(msg_res) = stream.next().await { let msg = msg_res.context("UID FETCH stream item")?; - out.push(fetch_to_list_entry(&msg)); + if let Some(entry) = fetch_to_list_entry(&msg) { + out.push(entry); + } } drop(stream); // Default to newest-first; callers (thread) re-sort if they want oldest-first. @@ -853,8 +884,8 @@ fn format_imap_since(iso_date: &str) -> Result { let y: u32 = parts[0].parse().context("year")?; let m: u32 = parts[1].parse().context("month")?; let d: u32 = parts[2].parse().context("day")?; - if !(1900..=9999).contains(&y) { - return Err(anyhow!("year out of range (1900..=9999)")); + if !(1970..=9999).contains(&y) { + return Err(anyhow!("year out of range (1970..=9999)")); } if !(1..=31).contains(&d) { return Err(anyhow!("day out of range (1..=31)")); diff --git a/crates/mail-mcp/src/smtp.rs b/crates/mail-mcp/src/smtp.rs index e12b0a7..78ef44a 100644 --- a/crates/mail-mcp/src/smtp.rs +++ b/crates/mail-mcp/src/smtp.rs @@ -17,10 +17,12 @@ use anyhow::{anyhow, Context, Result}; use base64::Engine; +use chrono::{SecondsFormat, Utc}; use lettre::message::header::ContentType; -use lettre::message::{Attachment, MultiPart, SinglePart}; +use lettre::message::{Attachment, Mailbox, MultiPart, SinglePart}; use lettre::transport::smtp::authentication::Credentials; use lettre::{AsyncSmtpTransport, AsyncTransport, Message, Tokio1Executor}; +use serde::Serialize; use crate::config::Account; @@ -44,7 +46,7 @@ pub struct SendInput { pub references: Vec, } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize)] pub struct SendOutput { pub message_id: String, pub sent_at: String, // RFC-3339 @@ -56,7 +58,13 @@ const USER_AGENT: &str = concat!("mail-mcp/", env!("CARGO_PKG_VERSION")); /// Match Gmail's effective 25 MB per-message ceiling — any single attachment /// past this is almost certainly unintended. Body caps are generous; HTML /// bodies past 5 MB are a smell. +/// +/// Note the asymmetry with imap.rs's MAX_RAW_EML_BYTES (20 MB) — we can +/// RECEIVE more than we'll let the LLM SEND because rspamd/postfix-relay +/// can rewrite + add headers; on the read side we want to refuse very +/// large messages before parsing them into RAM. const MAX_ATTACHMENT_BYTES: usize = 25 * 1024 * 1024; +const MAX_ATTACHMENT_BASE64_BYTES: usize = (MAX_ATTACHMENT_BYTES * 4 + 2) / 3; const MAX_ATTACHMENTS: usize = 25; const MAX_BODY_BYTES: usize = 5 * 1024 * 1024; const MAX_TOTAL_RECIPIENTS: usize = 100; @@ -97,9 +105,8 @@ pub fn validate_send_input(input: &SendInput) -> Result<()> { // Coarse pre-decode size estimate — base64 expands the payload ~33%, // so the encoded string is ~4*N/3 of the decoded bytes. Refuse early // if the encoded form alone exceeds 4/3 of MAX, saving a giant decode. - let max_encoded = (MAX_ATTACHMENT_BYTES * 4 + 2) / 3; for att in &input.attachments { - if att.content_base64.len() > max_encoded { + if att.content_base64.len() > MAX_ATTACHMENT_BASE64_BYTES { return Err(anyhow!( "attachment `{}` encoded length {} exceeds limit (decoded cap {MAX_ATTACHMENT_BYTES})", att.filename, @@ -110,13 +117,48 @@ pub fn validate_send_input(input: &SendInput) -> Result<()> { Ok(()) } +/// Strip an RFC-5322 mailbox down to the bare address, dropping any display +/// name. `Kayos ` -> `kayos@sulkta.com`; `kayos@sulkta.com` +/// passes through unchanged. Used by `mail_reply` because lettre's mailbox +/// parser is strict — we feed it bare addresses extracted from the original +/// `From` line rather than re-rendering names. +pub fn extract_bare_addr(s: &str) -> String { + if let (Some(lt), Some(gt)) = (s.find('<'), s.rfind('>')) { + if lt < gt { + return s[lt + 1..gt].trim().to_string(); + } + } + s.trim().to_string() +} + +/// Ensure a Message-ID-shaped value carries the canonical `` form. +/// `mail-parser` strips the brackets when it returns Message-Id / References +/// values; lettre's `in_reply_to()` / `references()` write the bytes through +/// verbatim, and strict RFC-5322 receivers will drop the threading link if +/// the brackets are missing. +pub fn ensure_angle_brackets(s: &str) -> String { + let trimmed = s.trim(); + if trimmed.starts_with('<') && trimmed.ends_with('>') { + trimmed.to_string() + } else { + format!("<{trimmed}>") + } +} + pub async fn send(account: &Account, input: SendInput) -> Result { validate_send_input(&input)?; // Build From, To, Cc, Bcc. - let from_str = format!("{} <{}>", account.from_name, account.from_addr); + // Use Mailbox::new directly instead of round-tripping through a string — + // if `from_name` ever contains `<` or `>`, the formatted-then-parsed path + // breaks unhelpfully. + let from_addr = account + .from_addr + .parse() + .with_context(|| format!("parse from address `{}`", account.from_addr))?; + let from_mb = Mailbox::new(Some(account.from_name.clone()), from_addr); let mut builder = Message::builder() - .from(from_str.parse().context("parse from address")?) + .from(from_mb) .subject(&input.subject); for addr in &input.to { @@ -174,7 +216,7 @@ pub async fn send(account: &Account, input: SendInput) -> Result { .build() }; - let sent_at = chrono_rfc3339_now(); + let sent_at = Utc::now().to_rfc3339_opts(SecondsFormat::Secs, true); transport .send(email) .await @@ -249,46 +291,6 @@ fn build_body(input: &SendInput) -> Result { Ok(mixed) } -/// Render `now` as RFC-3339 UTC (`2026-05-21T06:42:18Z`). We avoid pulling -/// the full `chrono` crate just for this; build the string by hand from -/// std time. -fn chrono_rfc3339_now() -> String { - use std::time::{SystemTime, UNIX_EPOCH}; - let secs = SystemTime::now() - .duration_since(UNIX_EPOCH) - .map(|d| d.as_secs()) - .unwrap_or(0); - // RFC-3339 via the same algorithm `httpdate` uses, simplified for UTC. - let (year, month, day, hour, minute, second) = civil_from_unix(secs as i64); - format!( - "{:04}-{:02}-{:02}T{:02}:{:02}:{:02}Z", - year, month, day, hour, minute, second - ) -} - -/// Convert a unix timestamp (UTC) to civil (Y,M,D,h,m,s). Copy of the -/// classic Howard Hinnant algorithm — works for the full proleptic Gregorian range. -fn civil_from_unix(t: i64) -> (i64, u32, u32, u32, u32, u32) { - let days = t.div_euclid(86_400); - let secs_of_day = t.rem_euclid(86_400); - let hour = (secs_of_day / 3600) as u32; - let minute = ((secs_of_day % 3600) / 60) as u32; - let second = (secs_of_day % 60) as u32; - - // 0000-03-01 is the start of the cycle ("era"). - let z = days + 719_468; - let era = if z >= 0 { z } else { z - 146_096 } / 146_097; - let doe = (z - era * 146_097) as u64; - let yoe = - (doe - doe / 1460 + doe / 36_524 - doe / 146_096) / 365; - let y = yoe as i64 + era * 400; - let doy = doe - (365 * yoe + yoe / 4 - yoe / 100); - let mp = (5 * doy + 2) / 153; - let d = (doy - (153 * mp + 2) / 5 + 1) as u32; - let m = if mp < 10 { (mp + 3) as u32 } else { (mp - 9) as u32 }; - let y = if m <= 2 { y + 1 } else { y }; - (y, m, d, hour, minute, second) -} #[cfg(test)] @@ -402,39 +404,54 @@ mod tests { assert!(validate_send_input(&i).is_ok()); } - // ----- civil_from_unix ----- + // ----- extract_bare_addr ----- #[test] - fn civil_from_unix_epoch() { - assert_eq!(civil_from_unix(0), (1970, 1, 1, 0, 0, 0)); - } - - #[test] - fn civil_from_unix_y2k() { - // 2000-01-01 00:00:00 UTC = 946684800 - assert_eq!(civil_from_unix(946_684_800), (2000, 1, 1, 0, 0, 0)); - } - - #[test] - fn civil_from_unix_2026_05_21() { - // 2026-05-21 13:44:03 UTC = 1779371043 + fn extract_bare_addr_strips_display_name() { assert_eq!( - civil_from_unix(1_779_371_043), - (2026, 5, 21, 13, 44, 3) + extract_bare_addr("Kayos "), + "kayos@sulkta.com" ); + assert_eq!( + extract_bare_addr("\"Cobb Hayes\" "), + "cobb@sulkta.com" + ); + assert_eq!(extract_bare_addr(" spaces "), "a@b.com"); } #[test] - fn civil_from_unix_pre_epoch() { - // 1969-12-31 23:59:59 UTC = -1 - assert_eq!(civil_from_unix(-1), (1969, 12, 31, 23, 59, 59)); - // 1969-01-01 00:00:00 UTC = -31536000 - assert_eq!(civil_from_unix(-31_536_000), (1969, 1, 1, 0, 0, 0)); + fn extract_bare_addr_passes_bare_address() { + assert_eq!(extract_bare_addr("plain@addr.com"), "plain@addr.com"); + assert_eq!(extract_bare_addr(" trim-me@x.com "), "trim-me@x.com"); } #[test] - fn civil_from_unix_leap_year() { - // 2024-02-29 00:00:00 UTC = 1709164800 (leap day) - assert_eq!(civil_from_unix(1_709_164_800), (2024, 2, 29, 0, 0, 0)); + fn extract_bare_addr_handles_garbage_gracefully() { + assert_eq!(extract_bare_addr("> a@b.com <"), "> a@b.com <"); + assert_eq!(extract_bare_addr(""), ""); + assert_eq!(extract_bare_addr("Just A Name"), "Just A Name"); + } + + // ----- ensure_angle_brackets ----- + + #[test] + fn ensure_angle_brackets_wraps_bare_id() { + assert_eq!(ensure_angle_brackets("abc@host"), ""); + assert_eq!(ensure_angle_brackets(" abc@host "), ""); + } + + #[test] + fn ensure_angle_brackets_preserves_wrapped_id() { + assert_eq!(ensure_angle_brackets(""), ""); + assert_eq!(ensure_angle_brackets(" "), ""); + } + + #[test] + fn ensure_angle_brackets_handles_partial() { + // One-sided wrap: code path is intentional — wraps the trimmed + // input again, which produces a doubled bracket. We're not the + // RFC-5322 parser; downstream lettre catches truly malformed + // values. This is the "best-effort" surface. + assert_eq!(ensure_angle_brackets(""); } } diff --git a/crates/mail-mcp/src/tools.rs b/crates/mail-mcp/src/tools.rs index 76471a7..33d2fcc 100644 --- a/crates/mail-mcp/src/tools.rs +++ b/crates/mail-mcp/src/tools.rs @@ -27,17 +27,29 @@ use crate::{imap as imap_mod, smtp as smtp_mod}; #[derive(Clone)] pub struct MailService { - inner: Arc, -} - -struct MailInner { - config: Config, + config: Arc, } impl MailService { pub fn new(config: Config) -> Self { Self { - inner: Arc::new(MailInner { config }), + config: Arc::new(config), + } + } +} + +/// Convert any `anyhow::Result` into the `Result` shape +/// rmcp tool methods must return. Renders the error chain via `{:#}` so the +/// `with_context` causes show up in the MCP `isError` content. +trait IntoMcpError { + fn to_mcp(self) -> Result; +} + +impl IntoMcpError for Result { + fn to_mcp(self) -> Result { + match self { + Ok(v) => serde_json::to_string(&v).map_err(|e| format!("serialize: {e:#}")), + Err(e) => Err(format!("{e:#}")), } } } @@ -259,7 +271,6 @@ impl MailService { #[tool(aggr)] args: SendArgs, ) -> Result { let account = self - .inner .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; @@ -282,14 +293,7 @@ impl MailService { in_reply_to: args.in_reply_to, references: args.references, }; - let out = smtp_mod::send(account, input) - .await - .map_err(|e| format!("{e:#}"))?; - serde_json::to_string(&serde_json::json!({ - "message_id": out.message_id, - "sent_at": out.sent_at, - })) - .map_err(|e| e.to_string()) + smtp_mod::send(account, input).await.to_mcp() } #[tool( @@ -301,7 +305,6 @@ impl MailService { #[tool(aggr)] args: ListArgs, ) -> Result { let account = self - .inner .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; @@ -329,7 +332,6 @@ impl MailService { #[tool(aggr)] args: FolderListArgs, ) -> Result { let account = self - .inner .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; @@ -348,7 +350,6 @@ impl MailService { #[tool(aggr)] args: SearchArgs, ) -> Result { let account = self - .inner .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; @@ -373,7 +374,6 @@ impl MailService { #[tool(aggr)] args: ThreadArgs, ) -> Result { let account = self - .inner .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; @@ -397,7 +397,6 @@ impl MailService { #[tool(aggr)] args: MoveArgs, ) -> Result { let account = self - .inner .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; @@ -421,7 +420,6 @@ impl MailService { #[tool(aggr)] args: MarkArgs, ) -> Result { let account = self - .inner .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; @@ -441,7 +439,6 @@ impl MailService { #[tool(aggr)] args: AttachmentGetArgs, ) -> Result { let account = self - .inner .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; @@ -465,7 +462,6 @@ impl MailService { #[tool(aggr)] args: ReplyArgs, ) -> Result { let account = self - .inner .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; @@ -485,7 +481,11 @@ impl MailService { let to: Vec = if let Some(overrides) = args.to_override { overrides } else { - original.from.iter().map(|s| extract_addr(s)).collect() + original + .from + .iter() + .map(|s| smtp_mod::extract_bare_addr(s)) + .collect() }; if to.is_empty() { return Err("no recipient — original has no From and no to_override given".into()); @@ -493,41 +493,48 @@ impl MailService { // Cc: reply_all -> original.cc; else empty. let cc: Vec = if args.reply_all { - original.cc.iter().map(|s| extract_addr(s)).collect() + original + .cc + .iter() + .map(|s| smtp_mod::extract_bare_addr(s)) + .collect() } else { vec![] }; - // Subject: keep an existing Re: prefix; otherwise add one. - let subject = if original + // Subject: keep an existing Re: prefix; otherwise add one. No alloc + // for the check — slice the first 3 bytes and compare case-insensitive. + let has_re_prefix = original .subject - .to_ascii_lowercase() - .starts_with("re:") - { + .get(..3) + .map(|s| s.eq_ignore_ascii_case("re:")) + .unwrap_or(false); + let subject = if has_re_prefix { original.subject.clone() } else { format!("Re: {}", original.subject) }; // Threading headers: In-Reply-To = parent Message-Id; References = - // parent References + parent Message-Id. - let parent_msgid = original - .message_id - .clone() - .ok_or("original message has no Message-Id — cannot thread reply")?; + // parent References + parent Message-Id. mail-parser strips angle + // brackets when it returns Message-Id; lettre passes through verbatim + // and strict RFC-5322 receivers will drop the threading link if the + // brackets are missing. Force the canonical form before sending. + let parent_msgid = smtp_mod::ensure_angle_brackets( + &original + .message_id + .clone() + .ok_or("original message has no Message-Id — cannot thread reply")?, + ); let mut references: Vec = Vec::new(); - // Mail-parser returns the References header (if any) via raw_header. - // Read it out of the flat headers map we already serialized. - if let Some(refs_value) = original.headers.get("References") { - if let Some(refs_str) = refs_value.as_str() { - references.extend( - refs_str - .split_whitespace() - .filter(|s| !s.is_empty()) - .map(|s| s.to_string()), - ); - } + if let Some(refs_str) = original.headers.get("References") { + references.extend( + refs_str + .split_whitespace() + .filter(|s| !s.is_empty()) + .map(smtp_mod::ensure_angle_brackets), + ); } references.push(parent_msgid.clone()); @@ -550,14 +557,7 @@ impl MailService { in_reply_to: Some(parent_msgid), references, }; - let out = smtp_mod::send(account, input) - .await - .map_err(|e| format!("{e:#}"))?; - serde_json::to_string(&serde_json::json!({ - "message_id": out.message_id, - "sent_at": out.sent_at, - })) - .map_err(|e| e.to_string()) + smtp_mod::send(account, input).await.to_mcp() } #[tool( @@ -569,7 +569,6 @@ impl MailService { #[tool(aggr)] args: ReadArgs, ) -> Result { let account = self - .inner .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; @@ -585,57 +584,17 @@ impl MailService { } } -/// Strip an RFC-5322 mailbox down to the bare address, dropping any display -/// name. `Kayos ` -> `kayos@sulkta.com`; `kayos@sulkta.com` -/// passes through unchanged. Used by `mail_reply` because lettre's address -/// parser wants either a bare addr or a full `"Name" ` shape. -fn extract_addr(s: &str) -> String { - if let (Some(lt), Some(gt)) = (s.find('<'), s.rfind('>')) { - if lt < gt { - return s[lt + 1..gt].trim().to_string(); - } - } - s.trim().to_string() -} - #[cfg(test)] mod tests { use super::*; - #[test] - fn extract_addr_strips_display_name() { - assert_eq!(extract_addr("Kayos "), "kayos@sulkta.com"); - assert_eq!( - extract_addr("\"Cobb Hayes\" "), - "cobb@sulkta.com" - ); - assert_eq!(extract_addr(" spaces "), "a@b.com"); - } - - #[test] - fn extract_addr_passes_bare_address() { - assert_eq!(extract_addr("plain@addr.com"), "plain@addr.com"); - assert_eq!(extract_addr(" trim-me@x.com "), "trim-me@x.com"); - } - - #[test] - fn extract_addr_handles_garbage_gracefully() { - // Reversed brackets — fall through to trimmed input - assert_eq!(extract_addr("> a@b.com <"), "> a@b.com <"); - // No brackets and no address — caller of extract_addr is - // responsible; we just don't panic. - assert_eq!(extract_addr(""), ""); - assert_eq!(extract_addr("Just A Name"), "Just A Name"); - } - #[test] fn to_field_collapses_to_vec() { - match (ToField::One("a@b.com".into())).into_vec() { - v => assert_eq!(v, vec!["a@b.com".to_string()]), - } - match (ToField::Many(vec!["a@b.com".into(), "c@d.com".into()])).into_vec() { - v => assert_eq!(v, vec!["a@b.com".to_string(), "c@d.com".to_string()]), - } + let v = ToField::One("a@b.com".into()).into_vec(); + assert_eq!(v, vec!["a@b.com".to_string()]); + + let v = ToField::Many(vec!["a@b.com".into(), "c@d.com".into()]).into_vec(); + assert_eq!(v, vec!["a@b.com".to_string(), "c@d.com".to_string()]); } }