diff --git a/.forgejo/workflows/email.yml b/.forgejo/workflows/email.yml index 3a35aee..6f6ca3f 100644 --- a/.forgejo/workflows/email.yml +++ b/.forgejo/workflows/email.yml @@ -158,62 +158,6 @@ 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. @@ -332,7 +276,7 @@ jobs: - name: The email domain test suite must not shrink run: | set -euo pipefail - FLOOR=95 + FLOOR=60 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 cc739c4..d3853e2 100644 --- a/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md +++ b/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md @@ -532,11 +532,6 @@ 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 @@ -663,12 +658,12 @@ Negative tests: remove `#[serde(skip)]` → A1 gate fails. Set | ID | Task | |---|---| -| ~~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..."`. | +| 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..."`. | --- 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 08635ba..d6f4ff6 100644 --- a/crates/apps/nigig-email/src/email_frame/pages/bulk.rs +++ b/crates/apps/nigig-email/src/email_frame/pages/bulk.rs @@ -1,6 +1,5 @@ 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! { @@ -82,11 +81,6 @@ 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 { @@ -137,50 +131,8 @@ 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(); - - // 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); - } - } - } + self.label(cx, ids!(send_status)).set_text(cx, "Sending..."); + 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 342f367..7d7818a 100644 --- a/crates/nigig-core/src/email_account.rs +++ b/crates/nigig-core/src/email_account.rs @@ -485,55 +485,6 @@ 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 deleted file mode 100644 index 9e9d0d8..0000000 --- a/crates/nigig-core/src/email_send.rs +++ /dev/null @@ -1,625 +0,0 @@ -//! 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 d59bc4c..d0545ec 100644 --- a/crates/nigig-core/src/email_store.rs +++ b/crates/nigig-core/src/email_store.rs @@ -432,13 +432,6 @@ 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 e922885..d192d8e 100644 --- a/crates/nigig-core/src/email_worker.rs +++ b/crates/nigig-core/src/email_worker.rs @@ -175,88 +175,17 @@ 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) { - // 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()))); + if let Err(msg) = validate_send(&config, &to, &subject, &body) { + Cx::post_action(EmailWorkerAction::SendResult(Err(msg))); 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)); }); } @@ -292,30 +221,11 @@ async fn send_email_impl( .from .parse() .map_err(|e| format!("Invalid from: {e}"))?; - // 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 + 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) .body(body.to_owned()) .map_err(|e| format!("Build error: {e}"))?; let creds = Credentials::new(config.username.clone(), config.password.expose().to_owned()); @@ -678,65 +588,6 @@ 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 21d1404..2d6a55f 100644 --- a/crates/nigig-core/src/lib.rs +++ b/crates/nigig-core/src/lib.rs @@ -20,7 +20,6 @@ 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;