From 5e1c63eeaa41d2c69c369629a29906ff7a13d578 Mon Sep 17 00:00:00 2001 From: Kayos Date: Thu, 21 May 2026 09:22:39 -0700 Subject: [PATCH] final-approval audit fixes: HIGH-1/2/3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the post-cleanup approval audit, all blockers before the rename to a real codename: HIGH-1: ReadOutput.headers map kept LAST occurrence of duplicate headers, not FIRST. Comment said 'keep the first occurrence' but the code used Message::header_raw(name) which internally does .iter().rev().find(...) — returns the last one. For load-bearing headers like References this is usually singular so the bug was latent, but an attacker who could inject a second References: line would have gotten to override the first one used by mail_reply for threading. Switched to parsed.headers_raw() which iterates in arrival order — first-occurrence guaranteed. HIGH-2: tokio-rustls default features pulled aws-lc-rs + aws-lc-sys into the dep tree even though we explicitly went ring-only on rustls. The default feature chain on tokio-rustls v0.26 enables 'aws_lc_rs' via rustls. Pinned tokio-rustls to default-features=false and the matching small feature set: logging, tls12, ring. Verified via `cargo tree` — no aws-lc-* in the build, single ring v0.17.14 shared between rustls + tokio-rustls. ~9s shorter cmake step in cold builds, smaller binary, no C-FFI crypto surface area. HIGH-3: IntoMcpError trait was introduced in the cleanup pass but applied at only 2 of 10 tools — the other 8 still used the manual .map_err(|e| format!('{e:#}'))? + serde_json::to_string chain. Maintenance trap. Applied to_mcp() at all 8 sites (mail_inbox_list, mail_folder_list, mail_search, mail_thread, mail_attachment_get, mail_inbox_read; mail_move and mail_mark stay with literal {"ok":true} returns — no value to serialize). Tool methods are now uniformly: imap_mod::xxx(...).await.to_mcp() or for the few that need pre-arg work, three lines instead of seven. Wire smoke verified — read on uid 34 returns the same 13 headers shape, no empties, all canonical fields populated. cargo test 31/31. Repo chain: 2240bf7 -> 4251f51 -> f4b3199 -> 6432a1f -> 54a1a6b -> 6fb63b0 -> f7e698b -> b681953 -> 7c8e246 -> this. --- Cargo.lock | 77 +----------------------------------- Cargo.toml | 5 ++- crates/mail-mcp/src/imap.rs | 28 ++++++------- crates/mail-mcp/src/tools.rs | 32 ++++++--------- 4 files changed, 30 insertions(+), 112 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e20e400..a661d59 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -107,28 +107,6 @@ version = "1.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c08606f8c3cbf4ce6ec8e28fb0014a2c086708fe954eaa885384a6165172e7e8" -[[package]] -name = "aws-lc-rs" -version = "1.17.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5ec2f1fc3ec205783a5da9a7e6c1509cc69dedf09a1949e412c1e18469326d00" -dependencies = [ - "aws-lc-sys", - "zeroize", -] - -[[package]] -name = "aws-lc-sys" -version = "0.41.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1a2f9779ce85b93ab6170dd940ad0169b5766ff848247aff13bb788b832fe3f4" -dependencies = [ - "cc", - "cmake", - "dunce", - "fs_extra", -] - [[package]] name = "base64" version = "0.21.7" @@ -166,8 +144,6 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a1dce859f0832a7d088c4f1119888ab94ef4b5d6795d1ce05afb7fe159d79f98" dependencies = [ "find-msvc-tools", - "jobserver", - "libc", "shlex", ] @@ -191,15 +167,6 @@ dependencies = [ "windows-link", ] -[[package]] -name = "cmake" -version = "0.1.58" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c0f78a02292a74a88ac736019ab962ece0bc380e3f977bf72e376c5d78ff0678" -dependencies = [ - "cc", -] - [[package]] name = "compression-codecs" version = "0.4.38" @@ -299,12 +266,6 @@ dependencies = [ "syn", ] -[[package]] -name = "dunce" -version = "1.0.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "92773504d58c093f6de2459af4af33faa518c13451eb8f2b5698ed3d36e7c813" - [[package]] name = "dyn-clone" version = "1.0.20" @@ -416,12 +377,6 @@ dependencies = [ "percent-encoding", ] -[[package]] -name = "fs_extra" -version = "1.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "42703706b716c37f96a77aea830392ad231f44c9e9a67872fa5548707e11b11c" - [[package]] name = "futures" version = "0.3.32" @@ -521,18 +476,6 @@ dependencies = [ "wasi", ] -[[package]] -name = "getrandom" -version = "0.3.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "899def5c37c4fd7b2664648c28120ecec138e4d395b459e5ca34f9cce2dd77fd" -dependencies = [ - "cfg-if", - "libc", - "r-efi 5.3.0", - "wasip2", -] - [[package]] name = "getrandom" version = "0.4.2" @@ -541,7 +484,7 @@ checksum = "0de51e6874e94e7bf76d726fc5d13ba782deca734ff60d5bb2fb2607c7406555" dependencies = [ "cfg-if", "libc", - "r-efi 6.0.0", + "r-efi", "wasip2", "wasip3", ] @@ -733,16 +676,6 @@ version = "1.0.18" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8f42a60cbdf9a97f5d2305f08a87dc4e09308d1276d28c869c684d7777685682" -[[package]] -name = "jobserver" -version = "0.1.34" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9afb3de4395d6b3e67a780b6de64b51c978ecf11cb9a462c66be7d4ca9039d33" -dependencies = [ - "getrandom 0.3.4", - "libc", -] - [[package]] name = "js-sys" version = "0.3.98" @@ -1080,12 +1013,6 @@ version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "478e0585659a122aa407eb7e3c0e1fa51b1d8a870038bd29f0cf4a8551eea972" -[[package]] -name = "r-efi" -version = "5.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "69cdb34c158ceb288df11e18b4bd39de994f6657d83847bdffdbd7f346754b0f" - [[package]] name = "r-efi" version = "6.0.0" @@ -1192,7 +1119,6 @@ version = "0.23.40" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ef86cd5876211988985292b91c96a8f2d298df24e75989a43a3c73f2d4d8168b" dependencies = [ - "aws-lc-rs", "log", "once_cell", "ring", @@ -1217,7 +1143,6 @@ version = "0.103.13" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "61c429a8649f110dddef65e2a5ad240f747e85f7758a6bccc7e5777bd33f756e" dependencies = [ - "aws-lc-rs", "ring", "rustls-pki-types", "untrusted", diff --git a/Cargo.toml b/Cargo.toml index 4576c4b..bd44ec4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -37,7 +37,10 @@ lettre = { version = "0.11", default-features = false, features = [ # IMAP — async-imap is tokio-native and supports UID-based addressing # (which we use throughout the API surface). async-imap = { version = "0.10", default-features = false, features = ["runtime-tokio"] } -tokio-rustls = "0.26" +# tokio-rustls default-features pulls in aws-lc-rs via rustls's default +# feature chain. We use `ring` exclusively (installed once in main.rs); +# turn off defaults and add back only the small pieces we want. +tokio-rustls = { version = "0.26", default-features = false, features = ["logging", "tls12", "ring"] } rustls = { version = "0.23", default-features = false, features = ["std", "tls12", "ring"] } rustls-pki-types = "1" webpki-roots = "0.26" diff --git a/crates/mail-mcp/src/imap.rs b/crates/mail-mcp/src/imap.rs index 47017ed..c299c20 100644 --- a/crates/mail-mcp/src/imap.rs +++ b/crates/mail-mcp/src/imap.rs @@ -386,23 +386,23 @@ pub async fn read( }) .collect(); - // 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). + // Flat name→raw-value map. + // + // mail-parser's `headers_raw()` returns `(name, raw_value)` pairs in + // ARRIVAL order — we use that directly so on duplicate headers we + // keep the FIRST occurrence (matching the comment). `header_raw(name)` + // looks the wrong way for this purpose: internally it does + // `iter().rev().find(...)` and returns the LAST instance, which would + // let an attacker who can inject a second `References:` line override + // the first one. The iterator avoids that footgun. let mut headers: BTreeMap = BTreeMap::new(); - for h in parsed.headers() { - let name = h.name(); + for (name, raw) in parsed.headers_raw() { if headers.contains_key(name) { - continue; // duplicate header — keep the first occurrence + continue; } - 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 trimmed = raw.trim(); + if !trimmed.is_empty() { + headers.insert(name.to_string(), trimmed.to_string()); } } diff --git a/crates/mail-mcp/src/tools.rs b/crates/mail-mcp/src/tools.rs index 33d2fcc..d03ed16 100644 --- a/crates/mail-mcp/src/tools.rs +++ b/crates/mail-mcp/src/tools.rs @@ -308,7 +308,7 @@ impl MailService { .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; - let entries = imap_mod::list( + imap_mod::list( account, imap_mod::ListOpts { since: args.since, @@ -317,10 +317,8 @@ impl MailService { folder: args.folder, }, ) - .await - .map_err(|e| format!("{e:#}"))?; - serde_json::to_string(&entries).map_err(|e| e.to_string()) + .to_mcp() } #[tool( @@ -335,10 +333,7 @@ impl MailService { .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; - let folders = imap_mod::list_folders(account) - .await - .map_err(|e| format!("{e:#}"))?; - serde_json::to_string(&folders).map_err(|e| e.to_string()) + imap_mod::list_folders(account).await.to_mcp() } #[tool( @@ -353,16 +348,14 @@ impl MailService { .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; - let entries = imap_mod::search( + imap_mod::search( account, &args.query, args.folder.as_deref(), args.limit, ) - .await - .map_err(|e| format!("{e:#}"))?; - serde_json::to_string(&entries).map_err(|e| e.to_string()) + .to_mcp() } #[tool( @@ -377,15 +370,14 @@ impl MailService { .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; - let entries = imap_mod::thread( + imap_mod::thread( account, &args.message_id, args.folder.as_deref(), args.limit, ) .await - .map_err(|e| format!("{e:#}"))?; - serde_json::to_string(&entries).map_err(|e| e.to_string()) + .to_mcp() } #[tool( @@ -442,15 +434,14 @@ impl MailService { .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; - let out = imap_mod::attachment_get( + imap_mod::attachment_get( account, args.uid, args.folder.as_deref(), args.attachment_index as usize, ) .await - .map_err(|e| format!("{e:#}"))?; - serde_json::to_string(&out).map_err(|e| e.to_string()) + .to_mcp() } #[tool( @@ -572,15 +563,14 @@ impl MailService { .config .account(args.account.as_deref()) .map_err(|e| e.to_string())?; - let out = imap_mod::read( + imap_mod::read( account, args.uid, args.folder.as_deref(), args.format.as_deref().unwrap_or("text"), ) .await - .map_err(|e| format!("{e:#}"))?; - serde_json::to_string(&out).map_err(|e| e.to_string()) + .to_mcp() } }