audit-fix round 2: LOW-3, LOW-5, LOW-6

- LOW-3 (canonical Flag display): render_flag() pattern-matches the
  async-imap Flag enum to its IMAP wire syntax — \\Seen, \\Flagged,
  \\Deleted, etc. — instead of Debug syntax ("Seen", Custom("...")).
  Consumers checking for \\Seen now match.
- LOW-5 (schema 0=default sentinel): limit fields are now Option<u32>
  instead of bare u32 with a 0-means-default contract. JSON schema
  output is clearer; clamp_limit() still treats Some(0) the same as
  None for backwards compatibility.
- LOW-6 (config chmod gate): Config::load() now refuses to read a
  config file with group/other read bits set. Same posture as
  ssh-keygen rejecting loose private-key permissions. Refuses 0644
  cleanly; accepts 0600. Unix-only — Windows path is a no-op.

Smoke verified: loose-chmod test refuses to start with the expected
error; tight-chmod test starts and serves initialize cleanly. All
seven tools still listed with valid input schemas.
This commit is contained in:
Kayos 2026-05-21 07:55:43 -07:00
parent f4b3199e86
commit 6432a1f5ff
3 changed files with 69 additions and 26 deletions

View file

@ -47,6 +47,7 @@ fn default_true() -> bool {
impl Config {
pub fn load() -> Result<Self> {
let path = config_path()?;
check_chmod(&path)?;
let text = std::fs::read_to_string(&path)
.with_context(|| format!("read config {}", path.display()))?;
let cfg: Self = toml::from_str(&text)
@ -133,6 +134,31 @@ fn strip_quotes(s: &str) -> &str {
s
}
/// Defense-in-depth chmod check on the config file. Refuse to load anything
/// readable by group or other — the config carries password_env / password_file
/// pointers; even paths-to-secrets shouldn't be world-readable. Same posture
/// as ssh-keygen complaining about loose permissions on a private key.
#[cfg(unix)]
fn check_chmod(path: &std::path::Path) -> Result<()> {
use std::os::unix::fs::PermissionsExt;
let meta = std::fs::metadata(path)
.with_context(|| format!("stat config {}", path.display()))?;
let mode = meta.permissions().mode() & 0o777;
if mode & 0o077 != 0 {
return Err(anyhow!(
"config {} has loose permissions {:o} — must be 0600 (group/other bits not allowed)",
path.display(),
mode
));
}
Ok(())
}
#[cfg(not(unix))]
fn check_chmod(_path: &std::path::Path) -> Result<()> {
Ok(()) // Windows / other: skip — no comparable concept.
}
fn config_path() -> Result<PathBuf> {
if let Ok(p) = std::env::var("MAIL_MCP_CONFIG") {
return Ok(PathBuf::from(shellexpand::tilde(&p).into_owned()));

View file

@ -10,7 +10,7 @@
use std::sync::Arc;
use anyhow::{anyhow, Context, Result};
use async_imap::types::Fetch;
use async_imap::types::{Fetch, Flag};
use futures::StreamExt;
use mail_parser::{MessageParser, MimeHeaders};
use rustls::pki_types::ServerName;
@ -24,10 +24,25 @@ use crate::config::Account;
pub struct ListOpts {
pub since: Option<String>, // YYYY-MM-DD
pub unread_only: bool,
pub limit: u32, // 0 means default (50)
pub limit: Option<u32>, // None → DEFAULT_LIMIT
pub folder: Option<String>, // None → INBOX
}
/// Render an async-imap `Flag` as its canonical IMAP wire string:
/// `\Seen`, `\Flagged`, etc., or `$Custom` / `keyword` for user flags.
fn render_flag(f: &Flag<'_>) -> String {
match f {
Flag::Seen => "\\Seen".into(),
Flag::Answered => "\\Answered".into(),
Flag::Flagged => "\\Flagged".into(),
Flag::Deleted => "\\Deleted".into(),
Flag::Draft => "\\Draft".into(),
Flag::Recent => "\\Recent".into(),
Flag::MayCreate => "\\MayCreate".into(),
Flag::Custom(s) => s.to_string(),
}
}
#[derive(Debug, Clone, Serialize)]
pub struct ListEntry {
pub uid: u32,
@ -79,6 +94,17 @@ const MAX_LIMIT: u32 = 500;
/// but doesn't double-buffer through `String::from_utf8_lossy`).
const MAX_RAW_EML_BYTES: u64 = 20 * 1024 * 1024;
/// Clamp a caller-provided limit to `[1, MAX_LIMIT]` with `DEFAULT_LIMIT`
/// when omitted. Returning u32 since IMAP UID truncation works on count.
fn clamp_limit(limit: Option<u32>) -> u32 {
match limit {
None => DEFAULT_LIMIT,
Some(0) => DEFAULT_LIMIT,
Some(n) if n > MAX_LIMIT => MAX_LIMIT,
Some(n) => n,
}
}
/// 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
@ -111,11 +137,7 @@ fn validate_mailbox(name: &str) -> Result<()> {
pub async fn list(account: &Account, opts: ListOpts) -> Result<Vec<ListEntry>> {
let folder = opts.folder.as_deref().unwrap_or("INBOX");
validate_mailbox(folder)?;
let limit = match opts.limit {
0 => DEFAULT_LIMIT,
n if n > MAX_LIMIT => MAX_LIMIT,
n => n,
};
let limit = clamp_limit(opts.limit);
let mut session = open_session(account).await?;
session
@ -179,7 +201,7 @@ pub async fn list(account: &Account, opts: ListOpts) -> Result<Vec<ListEntry>> {
fn fetch_to_list_entry(msg: &Fetch) -> ListEntry {
let uid = msg.uid.unwrap_or(0);
let flags: Vec<String> = msg.flags().map(|f| format!("{f:?}")).collect();
let flags: Vec<String> = msg.flags().map(|f| render_flag(&f)).collect();
let header_bytes = msg.header().unwrap_or(&[]);
let parser = MessageParser::default();
@ -409,7 +431,7 @@ pub async fn search(
account: &Account,
query: &str,
folder: Option<&str>,
limit: u32,
limit: Option<u32>,
) -> Result<Vec<ListEntry>> {
if query.trim().is_empty() {
return Err(anyhow!("search query is empty"));
@ -431,11 +453,7 @@ pub async fn search(
}
let folder = folder.unwrap_or("INBOX");
validate_mailbox(folder)?;
let limit = match limit {
0 => DEFAULT_LIMIT,
n if n > MAX_LIMIT => MAX_LIMIT,
n => n,
};
let limit = clamp_limit(limit);
let mut session = open_session(account).await?;
session
@ -467,7 +485,7 @@ pub async fn thread(
account: &Account,
message_id: &str,
folder: Option<&str>,
limit: u32,
limit: Option<u32>,
) -> Result<Vec<ListEntry>> {
let id_unbraced = strip_msgid_braces(message_id);
if id_unbraced.is_empty() {
@ -485,11 +503,7 @@ pub async fn thread(
}
let folder = folder.unwrap_or("INBOX");
validate_mailbox(folder)?;
let limit = match limit {
0 => DEFAULT_LIMIT,
n if n > MAX_LIMIT => MAX_LIMIT,
n => n,
};
let limit = clamp_limit(limit);
let mut session = open_session(account).await?;
session

View file

@ -111,9 +111,10 @@ pub struct ListArgs {
/// If true, only list messages without the \Seen flag.
#[serde(default)]
pub unread_only: bool,
/// Max entries to return — default 50, max 500.
/// Max entries to return — default 50, max 500. Omit for default; 0
/// also means default (treated the same as None).
#[serde(default)]
pub limit: u32,
pub limit: Option<u32>,
/// IMAP folder. Default `INBOX`.
#[serde(default)]
pub folder: Option<String>,
@ -136,9 +137,9 @@ pub struct SearchArgs {
/// IMAP folder. Default `INBOX`.
#[serde(default)]
pub folder: Option<String>,
/// Max entries — default 50, max 500.
/// Max entries — default 50, max 500. Omit for default; 0 also means default.
#[serde(default)]
pub limit: u32,
pub limit: Option<u32>,
}
#[derive(Debug, Deserialize, schemars::JsonSchema)]
@ -151,9 +152,9 @@ pub struct ThreadArgs {
/// IMAP folder. Default `INBOX`.
#[serde(default)]
pub folder: Option<String>,
/// Max entries — default 50, max 500.
/// Max entries — default 50, max 500. Omit for default; 0 also means default.
#[serde(default)]
pub limit: u32,
pub limit: Option<u32>,
}
#[derive(Debug, Deserialize, schemars::JsonSchema)]
@ -254,6 +255,7 @@ impl MailService {
folder: args.folder,
},
)
.await
.map_err(|e| format!("{e:#}"))?;
serde_json::to_string(&entries).map_err(|e| e.to_string())
@ -297,6 +299,7 @@ impl MailService {
args.folder.as_deref(),
args.limit,
)
.await
.map_err(|e| format!("{e:#}"))?;
serde_json::to_string(&entries).map_err(|e| e.to_string())