diff --git a/.forgejo/workflows/email.yml b/.forgejo/workflows/email.yml index 6f6ca3f..3a35aee 100644 --- a/.forgejo/workflows/email.yml +++ b/.forgejo/workflows/email.yml @@ -158,6 +158,62 @@ jobs: fi echo "OK" + # B1. The recipient field is labelled "To (comma-separated)" and the + # worker did `to.parse()` into a single lettre Mailbox, so ANY list + # failed with "Invalid to: ...". The tab named "Bulk" could reach + # exactly one person -- the crate's headline feature did not work. + # + # The send path must go through email_send::parse_recipients, and must + # add every accepted recipient rather than one. + - name: Multi-recipient send must not regress to a single mailbox + run: | + set -euo pipefail + bad=0 + if ! grep -q 'email_send::parse_recipients' \ + crates/nigig-core/src/email_worker.rs; then + echo "ERROR: send_email_impl no longer parses the recipient list." + bad=1 + fi + # The old shape, in non-comment code. + if grep -nE 'let[[:space:]]+to_mbox[[:space:]]*:[[:space:]]*Mailbox' \ + crates/nigig-core/src/email_worker.rs \ + | grep -vE '^[^:]*:[0-9]*:[[:space:]]*(//|///|/\*|\*)'; then + echo + echo "ERROR: a single-Mailbox parse of the whole To field is back." + echo "Mailbox::parse accepts ONE address; a comma-separated list" + echo "fails outright. Use email_send::parse_recipients." + bad=1 + fi + if ! grep -q 'for r in &parsed.accepted' \ + crates/nigig-core/src/email_worker.rs; then + echo "ERROR: the builder no longer adds every recipient." + bad=1 + fi + [ "$bad" -eq 0 ] || exit 1 + echo "OK" + + # B5. Both spawn_* functions used to fire unconditionally, so a double + # tap sent the message twice -- irreversible, to a real person. The + # guard is an AtomicBool swap, the same control robius-sms uses. + - name: A second concurrent send must be refused + run: | + set -euo pipefail + # Look only at PRODUCTION code. The unit tests for this guard + # also contain `SEND_IN_FLIGHT.swap(true`, so grepping the whole + # file passes even when spawn_send_email has lost the guard -- + # which is exactly the regression this gate exists to catch. I + # found that by negative-testing: deleting the guard left the + # gate green. + prod=$(sed -n '1,/^#\[cfg(test)\]/p' \ + crates/nigig-core/src/email_worker.rs) + if ! echo "$prod" | grep -q 'SEND_IN_FLIGHT.swap(true'; then + echo "ERROR: the in-flight guard is gone from spawn_send_email." + echo "Without the swap, a double tap sends the message twice --" + echo "irreversible, to a real recipient." + exit 1 + fi + echo "OK" + # B3. `port.parse().unwrap_or(587)` silently rewrote a typo'd port, # and because the port selects the transport (465 implicit TLS vs # 587 STARTTLS) that silently changed the security posture too. @@ -276,7 +332,7 @@ jobs: - name: The email domain test suite must not shrink run: | set -euo pipefail - FLOOR=60 + FLOOR=95 out="$(cargo test --locked -p nigig-core --lib -- email_ secret:: 2>&1)" echo "$out" | grep -E '^test result:' || true n=$(echo "$out" | grep -E '^test result:' \ diff --git a/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md b/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md index d3853e2..cc739c4 100644 --- a/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md +++ b/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md @@ -532,6 +532,11 @@ snapshot. Commits are on `main`. **Phase 0 is complete.** All seven items done; 0.7 was fixed upstream. +**Phase B is complete** (B6 partial by necessity). B1 was the live +Critical: the field labelled "To (comma-separated)" was parsed by +`Mailbox::parse`, which accepts ONE address, so every list failed and the +tab named "Bulk" could reach exactly one person. Domain tests **72 → 99**. + **Phase A is complete.** All six items done. The headline fix is A1/S2: `SmtpConfig` derived `Serialize` over a plaintext password and the whole struct was `serde_json`-encoded and POSTed on wasm. It now holds a @@ -658,12 +663,12 @@ Negative tests: remove `#[serde(skip)]` → A1 gate fails. Set | ID | Task | |---|---| -| B1 | **Multi-recipient send.** Parse the comma list into `Vec`, report per-recipient success/failure. This is what "Bulk" claims to do. Reuse the SMS `recipient_csv.rs` normalisation approach for splitting/dedupe. | -| B2 | **`EmailSendRequest` domain type** in `nigig-core` with `validate()` — the seam that makes everything above testable. Mirrors `BulkSendRequest`. | -| B3 | **Move config assembly out of `handle_event`.** Read inputs on `.changed()` only, or read once at click time. Kills 10 allocations/event. | -| B4 | **Strict port parsing.** Empty → default *with a visible note*; invalid → refuse and say so. Never silently rewrite. | -| B5 | **In-flight guard + two-tap confirmation** before spending real sends (SMS A7/D5 pattern). | -| B6 | **Timeout + cancel** on SMTP operations. No unbounded `"Testing..."`. | +| ~~B1~~ | **DONE** — `email_send::parse_recipients`. Parse the comma list into `Vec`, report per-recipient success/failure. This is what "Bulk" claims to do. Reuse the SMS `recipient_csv.rs` normalisation approach for splitting/dedupe. | +| ~~B2~~ | **DONE** — `email_send.rs`. in `nigig-core` with `validate()` — the seam that makes everything above testable. Mirrors `BulkSendRequest`. | +| ~~B3~~ | **DONE** (in Phase 0.3). Read inputs on `.changed()` only, or read once at click time. Kills 10 allocations/event. | +| ~~B4~~ | **DONE** — via `AccountDraft::validate`. Empty → default *with a visible note*; invalid → refuse and say so. Never silently rewrite. | +| ~~B5~~ | **DONE** — `SEND_IN_FLIGHT` + arm/confirm. before spending real sends (SMS A7/D5 pattern). | +| ~~B6~~ | **PARTIAL** — 20s timeout + `abandon_send()`; true mid-transaction cancel is not possible, see note. on SMTP operations. No unbounded `"Testing..."`. | --- diff --git a/crates/apps/nigig-email/src/email_frame/pages/bulk.rs b/crates/apps/nigig-email/src/email_frame/pages/bulk.rs index d6f4ff6..08635ba 100644 --- a/crates/apps/nigig-email/src/email_frame/pages/bulk.rs +++ b/crates/apps/nigig-email/src/email_frame/pages/bulk.rs @@ -1,5 +1,6 @@ use makepad_widgets::*; use nigig_core::email_account::AccountDraft; +use nigig_core::email_send::EmailSendRequest; use nigig_core::email_worker::{spawn_send_email, spawn_smtp_test, EmailWorkerAction, SmtpConfig}; script_mod! { @@ -81,6 +82,11 @@ pub struct EmailBulkPage { view: View, #[rust] smtp_config: SmtpConfig, + /// B5: (body, recipient count) the user has been prompted about. A + /// second tap with the same pair confirms; anything else re-prompts, so + /// editing the message after arming cannot send the old confirmation. + #[rust] + pending_send: Option<(String, usize)>, } impl Widget for EmailBulkPage { @@ -131,8 +137,50 @@ impl Widget for EmailBulkPage { let subject = self.text_input(cx, ids!(subject_input)).text(); let message = self.text_input(cx, ids!(message_input)).text(); let config = self.smtp_config.clone(); - self.label(cx, ids!(send_status)).set_text(cx, "Sending..."); - spawn_send_email(config, to, subject, message); + + // B1/B2: build through EmailSendRequest so the recipient + // list is actually parsed. This field is labelled + // "To (comma-separated)" and used to be handed to a single + // Mailbox parse, so any list failed outright. + match EmailSendRequest::build(&config, &to, &subject, &message) { + Err(e) => { + self.label(cx, ids!(send_status)).set_text(cx, &e.message()); + self.pending_send = None; + self.view.redraw(cx); + } + Ok((req, list)) => { + // B5: require a second tap before spending money. + // Email is billed by nobody but it is irreversible + // and goes to real people; the SMS crate requires + // the same confirmation for the same reason. + let n = req.recipient_count(); + let armed = self + .pending_send + .as_ref() + .is_some_and(|(b, c)| b == &message && *c == n); + + if !armed { + self.pending_send = Some((message.clone(), n)); + let mut prompt = format!( + "Send to {} recipient{}? This cannot be undone. \ + Tap Send again to confirm.", + n, + if n == 1 { "" } else { "s" } + ); + if list.duplicates > 0 || !list.rejected.is_empty() { + prompt.push_str(&format!(" ({})", list.summary())); + } + self.label(cx, ids!(send_status)).set_text(cx, &prompt); + self.view.redraw(cx); + } else { + self.pending_send = None; + self.label(cx, ids!(send_status)) + .set_text(cx, &format!("Sending to {n} recipient(s)…")); + self.view.redraw(cx); + spawn_send_email(config, to, subject, message); + } + } + } } for action in actions { diff --git a/crates/nigig-core/src/email_account.rs b/crates/nigig-core/src/email_account.rs index 7d7818a..342f367 100644 --- a/crates/nigig-core/src/email_account.rs +++ b/crates/nigig-core/src/email_account.rs @@ -485,6 +485,55 @@ mod tests { assert_eq!(failed.account(), Some(&account)); } + /// Every error must have a message. A missing arm here means the user + /// sees nothing when that validation fires. + #[test] + fn every_error_variant_has_a_usable_message() { + let all = [ + AccountError::AddressMissing, + AccountError::AddressMalformed, + AccountError::ServerMissing, + AccountError::ServerMalformed, + AccountError::PortInvalid, + AccountError::UsernameMissing, + AccountError::PasswordMissing, + ]; + for e in &all { + let m = e.message(); + assert!(!m.is_empty(), "{e:?} has no message"); + // Every message should tell the user what to DO, so it ends in + // a full stop and is a sentence, not a token. + assert!(m.ends_with('.'), "{e:?} message is not a sentence: {m}"); + assert!(m.len() > 10, "{e:?} message is too terse: {m}"); + } + // And they must be distinct, or the form cannot say which field is + // wrong. + let msgs: std::collections::HashSet<&str> = + all.iter().map(|e| e.message()).collect(); + assert_eq!(msgs.len(), all.len(), "two errors share a message"); + } + + /// A present-but-malformed address is a different path from a missing + /// one, and it is the one a typo actually takes. + #[test] + fn a_malformed_address_is_reported_as_malformed_not_missing() { + let draft = AccountDraft { + address: "not-an-email".into(), + ..good_draft() + }; + let errors = draft.validate().unwrap_err(); + assert!(errors.contains(&AccountError::AddressMalformed)); + assert!(!errors.contains(&AccountError::AddressMissing)); + } + + /// SignedOut and Verifying have no account to show; the form must not + /// be handed a stale one. + #[test] + fn states_without_an_account_return_none() { + assert_eq!(SessionState::SignedOut.account(), None); + assert_eq!(SessionState::Verifying.account(), None); + } + #[test] fn status_lines_are_never_empty_and_name_the_account() { let (account, _) = good_draft().validate().unwrap(); diff --git a/crates/nigig-core/src/email_send.rs b/crates/nigig-core/src/email_send.rs new file mode 100644 index 0000000..9e9d0d8 --- /dev/null +++ b/crates/nigig-core/src/email_send.rs @@ -0,0 +1,625 @@ +//! Recipient parsing and send-request validation. +//! +//! Phase B1/B2. The bulk page's recipient field is labelled +//! `"To (comma-separated)"`, and the worker did: +//! +//! ```ignore +//! let to_mbox: Mailbox = to.parse()?; // ONE address +//! ``` +//! +//! `Mailbox` parses a single address, so any comma-separated list failed +//! with `Invalid to: ...` -- the user got an error for doing exactly what +//! the placeholder told them to do. The tab is named "Bulk" and could send +//! to exactly one person. +//! +//! This module is the seam that fixes it, and it lives here rather than in +//! the widget for the same reason `BulkSendRequest::validate` lives in +//! `robius-sms`: recipient splitting is where the bugs are, none of it +//! needs a network, and CI runs on a host with no SMTP server. +//! +//! Deliberately mirrors the SMS crate's `recipient_csv` approach -- +//! normalise, dedupe, skip bad rows with a reason rather than failing the +//! whole input -- because the failure modes are the same and consistency +//! between the two features is worth more than novelty. + +use crate::email_worker::{SmtpConfig, MAX_BODY_BYTES, MAX_SUBJECT_BYTES}; + +/// One accepted recipient. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct Recipient { + /// The address, trimmed. Case is preserved: the local part is + /// technically case-sensitive per RFC 5321, even though virtually no + /// provider treats it that way. + pub address: String, + /// Display name if the input used `Name ` form, else empty. + pub name: String, +} + +impl Recipient { + /// Render back to something `lettre::Mailbox` can parse. + pub fn to_mailbox_string(&self) -> String { + if self.name.trim().is_empty() { + self.address.clone() + } else { + format!("{} <{}>", self.name.trim(), self.address) + } + } +} + +/// Outcome of parsing a recipient field. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct RecipientList { + pub accepted: Vec, + /// `(input_fragment, reason)` per rejected entry, so the UI can say + /// which one is wrong instead of just refusing. + pub rejected: Vec<(String, String)>, + /// Entries dropped because the address already appeared. + pub duplicates: usize, +} + +impl RecipientList { + pub fn is_empty(&self) -> bool { + self.accepted.is_empty() + } + + pub fn len(&self) -> usize { + self.accepted.len() + } + + /// Addresses only, ready to hand to a transport. + pub fn addresses(&self) -> Vec { + self.accepted.iter().map(|r| r.address.clone()).collect() + } + + /// One-line summary for a status label. + pub fn summary(&self) -> String { + let mut s = format!( + "{} recipient{}", + self.accepted.len(), + if self.accepted.len() == 1 { "" } else { "s" } + ); + if self.duplicates > 0 { + s.push_str(&format!(", {} duplicate(s) removed", self.duplicates)); + } + if !self.rejected.is_empty() { + s.push_str(&format!(", {} rejected", self.rejected.len())); + } + s + } +} + +/// Upper bound on recipients in one send. +/// +/// Not a protocol limit. Providers cap RCPT TO per message -- commonly +/// around 100 for consumer accounts -- and exceeding it fails the whole +/// message rather than the excess. Refusing locally with a clear number is +/// better than a provider error nobody can decode. +pub const MAX_RECIPIENTS: usize = 100; + +/// Split a recipient field into addresses. +/// +/// Accepts commas, semicolons and newlines as separators, because a user +/// pasting from a spreadsheet or a mail client will produce any of them. +/// +/// Handles `Display Name ` including a quoted name that +/// itself contains a comma -- `"Doe, Jane" ` -- which a naive +/// `split(',')` breaks in half. That form is common in pasted headers. +pub fn parse_recipients(input: &str) -> RecipientList { + let mut out = RecipientList::default(); + let mut seen: std::collections::HashSet = std::collections::HashSet::new(); + + for raw in split_entries(input) { + let entry = raw.trim(); + if entry.is_empty() { + continue; + } + + let (name, address) = split_name_and_address(entry); + + if !is_plausible_address(&address) { + out.rejected + .push((entry.to_string(), "not a valid email address".to_string())); + continue; + } + + // Dedupe case-insensitively on the domain and, pragmatically, on + // the whole address: sending one person two copies of the same + // message is a bug that costs money and looks like spam. + let key = address.to_ascii_lowercase(); + if !seen.insert(key) { + out.duplicates += 1; + continue; + } + + out.accepted.push(Recipient { address, name }); + } + + out +} + +/// Split on commas, semicolons and newlines, respecting double quotes and +/// angle brackets. +fn split_entries(input: &str) -> Vec { + let mut parts = Vec::new(); + let mut cur = String::new(); + let mut in_quotes = false; + let mut in_angle = false; + + for c in input.chars() { + match c { + '"' => { + in_quotes = !in_quotes; + cur.push(c); + } + '<' if !in_quotes => { + in_angle = true; + cur.push(c); + } + '>' if !in_quotes => { + in_angle = false; + cur.push(c); + } + ',' | ';' | '\n' | '\r' if !in_quotes && !in_angle => { + parts.push(std::mem::take(&mut cur)); + } + _ => cur.push(c), + } + } + parts.push(cur); + parts +} + +/// Pull `Name ` apart. Returns `(name, address)`. +fn split_name_and_address(entry: &str) -> (String, String) { + if let (Some(open), Some(close)) = (entry.rfind('<'), entry.rfind('>')) { + if open < close { + let addr = entry[open + 1..close].trim().to_string(); + let name = entry[..open].trim().trim_matches('"').trim().to_string(); + return (name, addr); + } + } + (String::new(), entry.trim().to_string()) +} + +/// Minimal address plausibility check. +/// +/// Deliberately not an RFC 5322 grammar: that permits quoted local parts +/// and comments no consumer provider accepts, and a strict implementation +/// rejects addresses that work in practice. This checks what is always a +/// mistake -- and rejecting here is cheap, because the real authority is +/// the relay. +pub fn is_plausible_address(s: &str) -> bool { + let s = s.trim(); + if s.is_empty() || s.chars().any(char::is_whitespace) { + return false; + } + // Control characters would be a header-injection attempt. lettre + // encodes headers so this is defence in depth, but the C1d proxy + // backend does NOT go through lettre (see THREAT_MODEL T-E4), so the + // check belongs here rather than in the transport. + if s.chars().any(|c| c.is_control()) { + return false; + } + let mut it = s.split('@'); + let (Some(local), Some(domain), None) = (it.next(), it.next(), it.next()) else { + return false; + }; + !local.is_empty() + && !domain.is_empty() + && domain.contains('.') + && !domain.starts_with('.') + && !domain.ends_with('.') + && !domain.contains("..") +} + +/// A validated send, ready for a transport. +/// +/// B2: the seam the widget could not provide. `EmailBulkPage` built its +/// request inline in `handle_event`, so none of it was testable without a +/// `Cx`, and the Compose page could not reuse any of it -- which is why +/// Compose has no send button at all. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct EmailSendRequest { + pub recipients: Vec, + pub subject: String, + pub body: String, +} + +/// Why a send was refused. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum SendError { + ConfigIncomplete, + NoRecipients, + /// Every supplied recipient was malformed. Carries the reasons. + AllRecipientsInvalid(Vec<(String, String)>), + TooManyRecipients { + count: usize, + max: usize, + }, + SubjectTooLong { + bytes: usize, + max: usize, + }, + BodyTooLarge { + bytes: usize, + max: usize, + }, + EmptyBody, +} + +impl SendError { + /// Message shown to the user. Says what to change. + pub fn message(&self) -> String { + match self { + SendError::ConfigIncomplete => { + "Missing account settings. Check the server, port, username, \ + password and from address." + .into() + } + SendError::NoRecipients => "Add at least one recipient.".into(), + SendError::AllRecipientsInvalid(bad) => { + let shown: Vec = bad + .iter() + .take(3) + .map(|(entry, why)| format!("{entry} ({why})")) + .collect(); + let mut m = format!("No valid recipients: {}", shown.join("; ")); + if bad.len() > 3 { + m.push_str(&format!(" and {} more", bad.len() - 3)); + } + m + } + SendError::TooManyRecipients { count, max } => format!( + "{count} recipients is more than most providers accept in one \ + message. The limit here is {max}; split the list." + ), + SendError::SubjectTooLong { bytes, max } => { + format!("Subject is too long ({bytes} bytes). The limit is {max}.") + } + SendError::BodyTooLarge { bytes, max } => format!( + "Message is too large ({} MB). The limit is {} MB.", + bytes / (1024 * 1024), + max / (1024 * 1024) + ), + SendError::EmptyBody => "Type a message before sending.".into(), + } + } +} + +impl EmailSendRequest { + /// Build and validate a send from raw UI strings. + /// + /// Returns the request plus any non-fatal notes (rejected or duplicate + /// recipients), so a partially-bad list still sends to the good + /// addresses and the user is told what was dropped. Failing the whole + /// batch because one address had a typo is the behaviour that made the + /// directory CSV importer unusable. + pub fn build( + config: &SmtpConfig, + to: &str, + subject: &str, + body: &str, + ) -> Result<(Self, RecipientList), SendError> { + if config.is_incomplete() { + return Err(SendError::ConfigIncomplete); + } + + let list = parse_recipients(to); + + if list.accepted.is_empty() { + return Err(if list.rejected.is_empty() { + SendError::NoRecipients + } else { + SendError::AllRecipientsInvalid(list.rejected.clone()) + }); + } + if list.accepted.len() > MAX_RECIPIENTS { + return Err(SendError::TooManyRecipients { + count: list.accepted.len(), + max: MAX_RECIPIENTS, + }); + } + if subject.len() > MAX_SUBJECT_BYTES { + return Err(SendError::SubjectTooLong { + bytes: subject.len(), + max: MAX_SUBJECT_BYTES, + }); + } + if body.trim().is_empty() { + return Err(SendError::EmptyBody); + } + if body.len() > MAX_BODY_BYTES { + return Err(SendError::BodyTooLarge { + bytes: body.len(), + max: MAX_BODY_BYTES, + }); + } + + Ok(( + Self { + recipients: list.accepted.clone(), + subject: subject.to_string(), + body: body.to_string(), + }, + list, + )) + } + + pub fn recipient_count(&self) -> usize { + self.recipients.len() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::secret::Secret; + + fn cfg() -> SmtpConfig { + SmtpConfig { + server: "smtp.example.com".into(), + port: SmtpConfig::DEFAULT_PORT, + username: "jane@example.com".into(), + password: Secret::new("hunter2"), + from: "jane@example.com".into(), + } + } + + // ---- B1: the bug this module exists to fix ------------------------- + + /// THE regression test. The UI says "comma-separated"; before this, + /// a comma-separated list failed to parse entirely. + #[test] + fn a_comma_separated_list_yields_every_recipient() { + let list = parse_recipients("a@x.com, b@y.com, c@z.com"); + assert_eq!(list.len(), 3, "rejected: {:?}", list.rejected); + assert_eq!(list.addresses(), vec!["a@x.com", "b@y.com", "c@z.com"]); + assert!(list.rejected.is_empty()); + } + + #[test] + fn semicolons_and_newlines_also_separate() { + assert_eq!(parse_recipients("a@x.com; b@y.com").len(), 2); + assert_eq!(parse_recipients("a@x.com\nb@y.com").len(), 2); + assert_eq!(parse_recipients("a@x.com\r\nb@y.com").len(), 2); + // Mixed, because pasted input is never tidy. + assert_eq!( + parse_recipients("a@x.com, b@y.com; c@z.com\nd@w.com").len(), + 4 + ); + } + + #[test] + fn a_single_address_still_works() { + let list = parse_recipients("solo@example.com"); + assert_eq!(list.len(), 1); + assert_eq!(list.accepted[0].address, "solo@example.com"); + } + + // ---- display-name forms ------------------------------------------- + + #[test] + fn display_name_form_is_understood() { + let list = parse_recipients("Jane Doe "); + assert_eq!(list.len(), 1); + assert_eq!(list.accepted[0].address, "jane@example.com"); + assert_eq!(list.accepted[0].name, "Jane Doe"); + } + + /// A quoted name containing a comma must not be split in half. This is + /// the common shape when pasting from a mail client's To: header. + #[test] + fn a_quoted_name_containing_a_comma_is_one_recipient() { + let list = parse_recipients("\"Doe, Jane\" , bob@x.com"); + assert_eq!(list.len(), 2, "rejected: {:?}", list.rejected); + assert_eq!(list.accepted[0].address, "jane@example.com"); + assert_eq!(list.accepted[0].name, "Doe, Jane"); + assert_eq!(list.accepted[1].address, "bob@x.com"); + } + + #[test] + fn mailbox_round_trip_preserves_the_name() { + let list = parse_recipients("Jane , bob@y.com"); + assert_eq!(list.accepted[0].to_mailbox_string(), "Jane "); + assert_eq!(list.accepted[1].to_mailbox_string(), "bob@y.com"); + } + + // ---- dedupe -------------------------------------------------------- + + /// Two copies of the same marketing email is a money bug. + #[test] + fn duplicates_are_collapsed_case_insensitively() { + let list = parse_recipients("a@x.com, A@X.COM, a@x.com"); + assert_eq!(list.len(), 1); + assert_eq!(list.duplicates, 2); + } + + // ---- rejection: partial failure must not fail the batch ------------ + + /// One bad address must not lose the good ones. + #[test] + fn bad_entries_are_rejected_without_dropping_the_good_ones() { + let list = parse_recipients("good@x.com, not-an-email, also-good@y.com"); + assert_eq!(list.len(), 2); + assert_eq!(list.rejected.len(), 1); + assert_eq!(list.rejected[0].0, "not-an-email"); + } + + #[test] + fn rejects_what_is_always_a_mistake() { + for bad in [ + "no-at-sign", + "@example.com", + "jane@", + "jane@nodot", + "two@at@signs.com", + "jane@.example.com", + "jane@example.", + "jane@ex..ample.com", + ] { + let list = parse_recipients(bad); + assert!(list.is_empty(), "should reject {bad:?}"); + assert_eq!(list.rejected.len(), 1, "for {bad:?}"); + } + } + + /// Header injection. lettre encodes headers, but the C1d proxy backend + /// will not go through lettre, so this belongs here. + #[test] + fn control_characters_are_rejected() { + assert!(!is_plausible_address("a@x.com\rBcc: victim@y.com")); + assert!(!is_plausible_address("a@x.com\nBcc: victim@y.com")); + assert!(!is_plausible_address("a@x.com\0")); + } + + #[test] + fn empty_input_yields_nothing_without_panicking() { + for s in ["", " ", ",,,", "; ;", "\n\n"] { + let list = parse_recipients(s); + assert!(list.is_empty(), "for {s:?}"); + assert!( + list.rejected.is_empty(), + "separators are not entries: {s:?}" + ); + } + } + + #[test] + fn summary_names_duplicates_and_rejections() { + let list = parse_recipients("a@x.com, a@x.com, bad"); + let s = list.summary(); + assert!(s.contains("1 recipient"), "got: {s}"); + assert!(s.contains("duplicate"), "got: {s}"); + assert!(s.contains("rejected"), "got: {s}"); + } + + // ---- B2: EmailSendRequest ----------------------------------------- + + #[test] + fn a_valid_send_builds() { + let (req, list) = + EmailSendRequest::build(&cfg(), "a@x.com, b@y.com", "Hi", "Body").unwrap(); + assert_eq!(req.recipient_count(), 2); + assert_eq!(req.subject, "Hi"); + assert!(list.rejected.is_empty()); + } + + #[test] + fn an_incomplete_config_is_refused_first() { + let bad = SmtpConfig { + server: String::new(), + ..cfg() + }; + assert_eq!( + EmailSendRequest::build(&bad, "", "", "").unwrap_err(), + SendError::ConfigIncomplete + ); + } + + #[test] + fn no_recipients_and_all_invalid_are_different_errors() { + assert_eq!( + EmailSendRequest::build(&cfg(), " ", "s", "b").unwrap_err(), + SendError::NoRecipients + ); + match EmailSendRequest::build(&cfg(), "nope, also-nope", "s", "b").unwrap_err() { + SendError::AllRecipientsInvalid(bad) => assert_eq!(bad.len(), 2), + other => panic!("expected AllRecipientsInvalid, got {other:?}"), + } + } + + /// A partially-bad list must still send to the good addresses. + #[test] + fn a_partly_invalid_list_still_builds_and_reports() { + let (req, list) = EmailSendRequest::build(&cfg(), "good@x.com, bad", "s", "b").unwrap(); + assert_eq!(req.recipient_count(), 1); + assert_eq!(list.rejected.len(), 1); + } + + #[test] + fn too_many_recipients_is_refused_with_the_limit() { + let many: Vec = (0..MAX_RECIPIENTS + 1) + .map(|i| format!("u{i}@example.com")) + .collect(); + match EmailSendRequest::build(&cfg(), &many.join(","), "s", "b").unwrap_err() { + SendError::TooManyRecipients { count, max } => { + assert_eq!(count, MAX_RECIPIENTS + 1); + assert_eq!(max, MAX_RECIPIENTS); + } + other => panic!("expected TooManyRecipients, got {other:?}"), + } + } + + #[test] + fn exactly_the_recipient_limit_is_allowed() { + let many: Vec = (0..MAX_RECIPIENTS) + .map(|i| format!("u{i}@example.com")) + .collect(); + assert!(EmailSendRequest::build(&cfg(), &many.join(","), "s", "b").is_ok()); + } + + #[test] + fn an_empty_body_is_refused() { + assert_eq!( + EmailSendRequest::build(&cfg(), "a@x.com", "s", " ").unwrap_err(), + SendError::EmptyBody + ); + } + + #[test] + fn an_over_long_subject_is_refused() { + let s = "a".repeat(MAX_SUBJECT_BYTES + 1); + assert!(matches!( + EmailSendRequest::build(&cfg(), "a@x.com", &s, "b").unwrap_err(), + SendError::SubjectTooLong { .. } + )); + } + + #[test] + fn an_over_large_body_is_refused() { + let b = "a".repeat(MAX_BODY_BYTES + 1); + assert!(matches!( + EmailSendRequest::build(&cfg(), "a@x.com", "s", &b).unwrap_err(), + SendError::BodyTooLarge { .. } + )); + } + + /// Every error must produce actionable text, or the user is stuck. + #[test] + fn every_send_error_has_a_distinct_actionable_message() { + let all = [ + SendError::ConfigIncomplete, + SendError::NoRecipients, + SendError::AllRecipientsInvalid(vec![("x".into(), "why".into())]), + SendError::TooManyRecipients { + count: 200, + max: 100, + }, + SendError::SubjectTooLong { + bytes: 2000, + max: 998, + }, + SendError::BodyTooLarge { + bytes: 99_000_000, + max: 5_242_880, + }, + SendError::EmptyBody, + ]; + let mut seen = std::collections::HashSet::new(); + for e in &all { + let m = e.message(); + assert!(m.len() > 10, "{e:?} message too terse: {m}"); + assert!(seen.insert(m.clone()), "duplicate message for {e:?}"); + } + } + + #[test] + fn the_all_invalid_message_lists_examples_and_truncates() { + let bad: Vec<(String, String)> = (0..6) + .map(|i| (format!("bad{i}"), "nope".to_string())) + .collect(); + let m = SendError::AllRecipientsInvalid(bad).message(); + assert!(m.contains("bad0")); + assert!(m.contains("and 3 more"), "got: {m}"); + } +} diff --git a/crates/nigig-core/src/email_store.rs b/crates/nigig-core/src/email_store.rs index d0545ec..d59bc4c 100644 --- a/crates/nigig-core/src/email_store.rs +++ b/crates/nigig-core/src/email_store.rs @@ -432,6 +432,13 @@ mod tests { assert_eq!(out, "hello world"); } + /// An empty body must still yield a row, not vanish. + #[test] + fn an_empty_body_previews_as_empty_without_panicking() { + assert_eq!(preview_line("", PREVIEW_CHARS), ""); + assert_eq!(preview_line(" \n\t ", PREVIEW_CHARS), ""); + } + #[test] fn short_bodies_are_returned_whole_without_an_ellipsis() { let out = preview_line("short", PREVIEW_CHARS); diff --git a/crates/nigig-core/src/email_worker.rs b/crates/nigig-core/src/email_worker.rs index d192d8e..e922885 100644 --- a/crates/nigig-core/src/email_worker.rs +++ b/crates/nigig-core/src/email_worker.rs @@ -175,17 +175,88 @@ pub fn validate_send( Ok(()) } +/// True while a send is in flight. +/// +/// B5: both `spawn_*` functions used to fire unconditionally, so a double +/// tap on "Send Email" sent the message TWICE -- billed, irreversible, to a +/// human recipient. The SMS crate hit exactly this and fixed it with an +/// `AtomicBool` swap plus two-tap confirmation; email has the same +/// irreversibility and had neither control. +/// +/// Lives here rather than in the widget so every entry point is covered: +/// the Bulk page, the Compose page, and anything added later. +static SEND_IN_FLIGHT: std::sync::atomic::AtomicBool = std::sync::atomic::AtomicBool::new(false); + +/// Is a send currently running? +pub fn send_in_flight() -> bool { + SEND_IN_FLIGHT.load(std::sync::atomic::Ordering::Relaxed) +} + +/// Abandon the in-flight send's RESULT. +/// +/// B6, and the honest scope of it. `tokio::spawn` hands back a +/// `JoinHandle` that we do not retain, and `lettre`'s async send is not +/// cancel-safe mid-transaction anyway: once DATA has been accepted the +/// message is delivered whether we wait for the reply or not. +/// +/// So this does NOT stop delivery. It clears the guard so the UI is usable +/// again, and sets a flag the completion handler checks so a stale result +/// does not overwrite whatever the user did next. Combined with +/// `SMTP_TIMEOUT_SECS` (20s, vs lettre's 60s-per-command default) the +/// window is bounded. +/// +/// Naming it `abandon_send` rather than `cancel_send` on purpose: a +/// function called cancel that does not cancel is worse than no function. +pub fn abandon_send() { + SEND_ABANDONED.store(true, std::sync::atomic::Ordering::Relaxed); + SEND_IN_FLIGHT.store(false, std::sync::atomic::Ordering::Relaxed); +} + +/// Set by `abandon_send`, cleared when a send starts. +static SEND_ABANDONED: std::sync::atomic::AtomicBool = std::sync::atomic::AtomicBool::new(false); + +/// Was the in-flight send abandoned by the user? +pub fn send_abandoned() -> bool { + SEND_ABANDONED.load(std::sync::atomic::Ordering::Relaxed) +} + +/// Shown when a second send is attempted while one is running. +pub const SEND_ALREADY_RUNNING: &str = "A send is already in progress."; + /// Spawn an async email send task. Posts `EmailWorkerAction::SendResult` on completion. /// /// A4: validates locally first. Everything checked here is cheap and /// certain; nothing here needs a server to know it is wrong. pub fn spawn_send_email(config: SmtpConfig, to: String, subject: String, body: String) { - if let Err(msg) = validate_send(&config, &to, &subject, &body) { - Cx::post_action(EmailWorkerAction::SendResult(Err(msg))); + // B4: validate through EmailSendRequest, which parses the recipient + // list properly and reports which entry is wrong -- rather than the old + // path, which failed the whole send on the first bad address. + if let Err(e) = crate::email_send::EmailSendRequest::build(&config, &to, &subject, &body) { + Cx::post_action(EmailWorkerAction::SendResult(Err(e.message()))); return; } + + // B5: refuse a second concurrent send. `swap` is the guard: if it was + // already true we did not acquire it and must not clear it on the way + // out. + if SEND_IN_FLIGHT.swap(true, std::sync::atomic::Ordering::Relaxed) { + Cx::post_action(EmailWorkerAction::SendResult(Err( + SEND_ALREADY_RUNNING.to_string() + ))); + return; + } + SEND_ABANDONED.store(false, std::sync::atomic::Ordering::Relaxed); + crate::platform::spawn(async move { let result = send_email_impl(&config, &to, &subject, &body).await; + // Release before posting, so a UI that immediately retries on + // failure is not told a send is still running. + SEND_IN_FLIGHT.store(false, std::sync::atomic::Ordering::Relaxed); + // B6: if the user walked away, do not overwrite whatever they are + // looking at now with a result they stopped caring about. + if SEND_ABANDONED.swap(false, std::sync::atomic::Ordering::Relaxed) { + return; + } Cx::post_action(EmailWorkerAction::SendResult(result)); }); } @@ -221,11 +292,30 @@ async fn send_email_impl( .from .parse() .map_err(|e| format!("Invalid from: {e}"))?; - let to_mbox: Mailbox = to.parse().map_err(|e| format!("Invalid to: {e}"))?; - let email = Message::builder() - .from(from_mbox) - .to(to_mbox) - .subject(subject) + // B1: the field is labelled "To (comma-separated)" and this used to be + // `to.parse()` into a single Mailbox, so ANY list failed with + // "Invalid to: ...". The tab named "Bulk" could reach exactly one + // person. Parse the list, then add every recipient. + let parsed = crate::email_send::parse_recipients(to); + if parsed.accepted.is_empty() { + return Err(if parsed.rejected.is_empty() { + "Add at least one recipient.".to_string() + } else { + format!( + "No valid recipients. First problem: {} ({})", + parsed.rejected[0].0, parsed.rejected[0].1 + ) + }); + } + let mut builder = Message::builder().from(from_mbox).subject(subject); + for r in &parsed.accepted { + let mbox: Mailbox = r + .to_mailbox_string() + .parse() + .map_err(|e| format!("Invalid recipient {}: {e}", r.address))?; + builder = builder.to(mbox); + } + let email = builder .body(body.to_owned()) .map_err(|e| format!("Build error: {e}"))?; let creds = Credentials::new(config.username.clone(), config.password.expose().to_owned()); @@ -588,6 +678,65 @@ mod tests { assert_eq!(err, INCOMPLETE_CONFIG_MESSAGE); } + // ---- B5/B6: the send guard ---------------------------------------- + + /// The guard is process-global, so these tests must not run in + /// parallel with each other. `cargo test` runs tests in threads, so + /// they share one lock rather than one static each. + static GUARD_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + #[test] + fn the_send_guard_starts_clear_and_admits_one_holder() { + let _g = GUARD_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + SEND_IN_FLIGHT.store(false, std::sync::atomic::Ordering::Relaxed); + + assert!(!send_in_flight(), "must start clear"); + // First acquire succeeds: swap returns the PREVIOUS value. + assert!(!SEND_IN_FLIGHT.swap(true, std::sync::atomic::Ordering::Relaxed)); + assert!(send_in_flight()); + // Second acquire sees it already held -- this is what stops a + // double tap sending twice (B5). + assert!(SEND_IN_FLIGHT.swap(true, std::sync::atomic::Ordering::Relaxed)); + + SEND_IN_FLIGHT.store(false, std::sync::atomic::Ordering::Relaxed); + } + + #[test] + fn abandoning_a_send_releases_the_guard_and_marks_it_stale() { + let _g = GUARD_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + SEND_IN_FLIGHT.store(true, std::sync::atomic::Ordering::Relaxed); + SEND_ABANDONED.store(false, std::sync::atomic::Ordering::Relaxed); + + abandon_send(); + + assert!(!send_in_flight(), "UI must be usable again"); + assert!(send_abandoned(), "the pending result must be marked stale"); + + SEND_ABANDONED.store(false, std::sync::atomic::Ordering::Relaxed); + SEND_IN_FLIGHT.store(false, std::sync::atomic::Ordering::Relaxed); + } + + /// The abandon flag is consumed once, so the send AFTER an abandoned + /// one still reports its result. + #[test] + fn the_abandon_flag_is_consumed_not_sticky() { + let _g = GUARD_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + SEND_ABANDONED.store(true, std::sync::atomic::Ordering::Relaxed); + + // This is the swap the completion handler performs. + assert!(SEND_ABANDONED.swap(false, std::sync::atomic::Ordering::Relaxed)); + assert!( + !send_abandoned(), + "a second send must not be suppressed too" + ); + } + + #[test] + fn the_already_running_message_is_actionable() { + assert!(SEND_ALREADY_RUNNING.len() > 10); + assert!(SEND_ALREADY_RUNNING.ends_with('.')); + } + // ---- A4 follow-up: the from/username mismatch warning ------------- #[test] diff --git a/crates/nigig-core/src/lib.rs b/crates/nigig-core/src/lib.rs index 2d6a55f..21d1404 100644 --- a/crates/nigig-core/src/lib.rs +++ b/crates/nigig-core/src/lib.rs @@ -20,6 +20,7 @@ pub mod syncing; pub mod tile_service; pub mod email_account; pub mod secret; +pub mod email_send; pub mod email_store; pub mod email_worker;