Sulkta-OSS/carrier
3
0
Fork 0
Code Issues 1 Pull requests 3 Projects Releases Packages Wiki Activity Actions

final-approval audit fixes: HIGH-1/2/3

Browse source 
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: cd04d2c -> b2716e0 -> 7cc31ef -> c2df959 -> acff1b7 ->
730fd01 -> f181e3a -> 9d1b228 -> 19b8f21 -> this.
This commit is contained in:
Sulkta 2026-05-21 09:22:39 -07:00
parent 19b8f2170e
commit bcf8a46713
4 changed files with 30 additions and 112 deletions
Download patch file Download diff file Expand all files Collapse all files

28
crates/mail-mcp/src/imap.rs
View file

@ -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<String, String> = 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());
}
}

32
crates/mail-mcp/src/tools.rs
View file

@ -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()
}
}