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.
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<String,String> 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<ListEntry> 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<MailInner> } to
MailService { config: Arc<Config> }. 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<T: Serialize>
-> Result<String, String>. 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.
