From d65f0cd3b8395d321447c1df0613498aab60aab2 Mon Sep 17 00:00:00 2001 From: andodeki Date: Mon, 17 Aug 2026 05:08:47 +0000 Subject: [PATCH 1/4] test(email): property-test the parsers (E2) proptest dev-dependency (the same one the SMS crate uses) and a new email_properties module pinning 'never panic + structural invariants' for the untrusted-input parsers: looks_like_email, looks_like_hostname, parse_recipients, is_plausible_address, preview_line (the A3 byte-offset class of bug) and parse_imap_date. Arbitrary input must not panic, and an accepted verdict must satisfy the structural checks it exists to enforce. --- Cargo.lock | 1 + crates/nigig-core/Cargo.toml | 5 ++ crates/nigig-core/src/email_properties.rs | 95 +++++++++++++++++++++++ crates/nigig-core/src/lib.rs | 3 + 4 files changed, 104 insertions(+) create mode 100644 crates/nigig-core/src/email_properties.rs diff --git a/Cargo.lock b/Cargo.lock index 411ebe0..4c53988 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2984,6 +2984,7 @@ dependencies = [ "matrix_client", "nigig-system-prefs", "postcard", + "proptest", "rand 0.8.7", "reqwest", "robius-directories", diff --git a/crates/nigig-core/Cargo.toml b/crates/nigig-core/Cargo.toml index 4115037..c767d04 100644 --- a/crates/nigig-core/Cargo.toml +++ b/crates/nigig-core/Cargo.toml @@ -59,6 +59,11 @@ async-net = { version = "2", optional = true } # C1f: a real platform keystore (feature `keystore`), native only. keyring = { version = "4", optional = true } +[dev-dependencies] +# E2: property-test the address/date parsers — never panic on arbitrary +# input. Same dev-dependency the SMS crate already uses. +proptest = "1" + [target.'cfg(target_arch = "wasm32")'.dependencies] wasm-bindgen = "0.2" wasm-bindgen-futures = "0.4" diff --git a/crates/nigig-core/src/email_properties.rs b/crates/nigig-core/src/email_properties.rs new file mode 100644 index 0000000..65b4dfb --- /dev/null +++ b/crates/nigig-core/src/email_properties.rs @@ -0,0 +1,95 @@ +//! Property tests for the email parsers (Phase E2). +//! +//! The address, hostname, recipient and date parsers all take *untrusted* +//! input — a `To` field, a server field, an `INTERNALDATE` header — so they +//! must never panic on arbitrary input, and their accept/reject decisions +//! must satisfy structural invariants. A unit test can only cover the +//! inputs its author happened to think of; these properties are the +//! guarantee that *any* input behaves. +//! +//! `proptest` is a dev-dependency (the same one the SMS crate uses), so +//! these run under `cargo test` and are picked up by the email domain +//! filter in CI. + +use proptest::prelude::*; + +use crate::email_account::{looks_like_email, looks_like_hostname}; +use crate::email_send::{is_plausible_address, parse_recipients}; +use crate::email_store::preview_line; +use crate::imap_client::parse_imap_date; + +proptest! { + /// `looks_like_email` never panics, and a `true` verdict implies the + /// structural properties the function exists to check: exactly one `@`, + /// non-empty local and domain, and a dot in the domain. + #[test] + fn looks_like_email_never_panics_and_is_structural(s in any::()) { + let verdict = looks_like_email(&s); + if verdict { + let t = s.trim(); + prop_assert_eq!( + t.matches('@').count(), + 1, + "an accepted address must have exactly one '@'" + ); + let (local, domain) = t.split_once('@').expect("one @ was asserted"); + prop_assert!(!local.is_empty(), "empty local part"); + prop_assert!(!domain.is_empty(), "empty domain"); + prop_assert!(domain.contains('.'), "dotless domain"); + } + } + + /// `looks_like_hostname` never panics, and a `true` verdict rejects the + /// things a hostname must not contain (a scheme, a path, whitespace). + #[test] + fn looks_like_hostname_never_panics_and_rejects_schemes(s in any::()) { + let verdict = looks_like_hostname(&s); + if verdict { + let t = s.trim(); + prop_assert!(!t.contains("://"), "scheme in accepted hostname"); + prop_assert!(!t.contains('/'), "path in accepted hostname"); + prop_assert!( + !t.chars().any(char::is_whitespace), + "whitespace in accepted hostname" + ); + } + } + + /// `parse_recipients` never panics, and every address it accepts is one + /// `is_plausible_address` also accepts — the parser must not be more + /// lenient than the plausibility check it is built on. + #[test] + fn parse_recipients_never_panics_and_accepts_only_plausible(s in any::()) { + let list = parse_recipients(&s); + for r in &list.accepted { + prop_assert!( + is_plausible_address(&r.address), + "accepted an implausible address" + ); + } + } + + /// `is_plausible_address` never panics on any input. + #[test] + fn is_plausible_address_never_panics(s in any::()) { + let _ = is_plausible_address(&s); + } + + /// `preview_line` never panics on any body and any limit. This is the + /// A3 class of bug — byte-offset slicing panics on multibyte text — and + /// the property is simply "no panic", on every body and every limit. + #[test] + fn preview_line_never_panics_on_any_body_and_limit( + body in any::(), + max in 0usize..2000usize, + ) { + let _ = preview_line(&body, max); + } + + /// `parse_imap_date` never panics on any input; it returns `Option`, + /// so garbage must simply be `None`, not a panic. + #[test] + fn parse_imap_date_never_panics_on_any_input(s in any::()) { + let _ = parse_imap_date(&s); + } +} diff --git a/crates/nigig-core/src/lib.rs b/crates/nigig-core/src/lib.rs index 12c3761..bc3225d 100644 --- a/crates/nigig-core/src/lib.rs +++ b/crates/nigig-core/src/lib.rs @@ -32,6 +32,9 @@ pub mod email_cache; pub mod imap_client; pub mod email_session; +#[cfg(test)] +mod email_properties; + pub use dir::app_data_dir; pub use persistence::*; From c51d448ba0a9bbe39fcdf18805fc28cb90b0c124 Mon Sep 17 00:00:00 2001 From: andodeki Date: Mon, 17 Aug 2026 05:08:53 +0000 Subject: [PATCH 2/4] test(email): SMTP sink integration test, and bound the whole send (E5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract build_email_message (the pure message construction) and send_bounded (the whole send wrapped in platform::timeout). A hand-rolled SMTP sink on 127.0.0.1 now receives a real send and asserts the envelope, every recipient, and the DATA payload — the first time the SMTP conversation has been executed in this repo. This surfaced a real defect: lettre's .timeout() only bounds the TCP connect, not the greeting/command reads, so a server that accepts and never greets hangs the send indefinitely (the review's A6/P4 '60s default' claim was wrong for the read path). send_bounded closes that gap, and a_send_to_a_silent_server_errors_instead_of_hanging pins it. --- crates/nigig-core/src/email_worker.rs | 282 ++++++++++++++++++++++++-- 1 file changed, 266 insertions(+), 16 deletions(-) diff --git a/crates/nigig-core/src/email_worker.rs b/crates/nigig-core/src/email_worker.rs index cdb69ba..3995ce1 100644 --- a/crates/nigig-core/src/email_worker.rs +++ b/crates/nigig-core/src/email_worker.rs @@ -474,21 +474,30 @@ use lettre::{ #[cfg(not(target_arch = "wasm32"))] async fn smtp_test_impl(config: &SmtpConfig) -> Result<(), String> { let mailer = acquire_transport(config)?; - mailer - .transport() - .test_connection() - .await - .map(|_| ()) - .map_err(|e| format!("SMTP test failed: {e}")) + // A6: bound the WHOLE operation, not just the TCP connect. lettre's + // `.timeout()` only bounds the connect; a server that accepts and then + // never greets (or never answers a command) would hang the read + // forever. `platform::timeout` closes that gap (E5 integration test + // `a_send_to_a_silent_server_errors_instead_of_hanging` pins it). + let outcome = crate::platform::timeout( + std::time::Duration::from_secs(SMTP_TIMEOUT_SECS), + mailer.transport().test_connection(), + ) + .await; + match outcome { + Ok(Ok(_)) => Ok(()), + Ok(Err(e)) => Err(format!("SMTP test failed: {e}")), + Err(_) => Err(format!("SMTP test timed out after {SMTP_TIMEOUT_SECS}s")), + } } #[cfg(not(target_arch = "wasm32"))] -async fn send_email_impl( +fn build_email_message( config: &SmtpConfig, to: &str, subject: &str, body: &str, -) -> Result<(), String> { +) -> Result { let from_mbox: Mailbox = config .from .parse() @@ -516,16 +525,48 @@ async fn send_email_impl( .map_err(|e| format!("Invalid recipient {}: {e}", r.address))?; builder = builder.to(mbox); } - let email = builder + builder .body(body.to_owned()) - .map_err(|e| format!("Build error: {e}"))?; + .map_err(|e| format!("Build error: {e}")) +} + +/// Send one message, bounded by an explicit timeout. +/// +/// A6: the whole send — connect AND the SMTP command reads — is bounded. +/// lettre's own `.timeout()` only covers the TCP connect, so a server that +/// accepts and then never greets (or never answers a command) would hang +/// the read forever. This is the one place that guarantee is made, so both +/// the production path and the integration test (`a_send_to_a_silent_…`) +/// go through it. +#[cfg(not(target_arch = "wasm32"))] +async fn send_bounded( + transport: &AsyncSmtpTransport, + email: Message, + timeout: std::time::Duration, +) -> Result<(), String> { + let outcome = crate::platform::timeout(timeout, transport.send(email)).await; + match outcome { + Ok(Ok(_)) => Ok(()), + Ok(Err(e)) => Err(format!("Send failed: {e}")), + Err(_) => Err(format!("Send timed out after {}s", timeout.as_secs())), + } +} + +#[cfg(not(target_arch = "wasm32"))] +async fn send_email_impl( + config: &SmtpConfig, + to: &str, + subject: &str, + body: &str, +) -> Result<(), String> { + let email = build_email_message(config, to, subject, body)?; let mailer = acquire_transport(config)?; - mailer - .transport() - .send(email) - .await - .map_err(|e| format!("Send failed: {e}"))?; - Ok(()) + send_bounded( + mailer.transport(), + email, + std::time::Duration::from_secs(SMTP_TIMEOUT_SECS), + ) + .await } /// Which TLS mode a port implies. @@ -1239,4 +1280,213 @@ mod tests { fn protocol_relative_urls_are_rejected() { assert!(!email_api_url_is_safe("//mail.example.com/api")); } + + // ---- E5: a real SMTP conversation against a local sink ------------- + // + // This is the first time the SMTP path is *executed* in this repo + // (the assessment's "What I have not verified" listed it). A minimal + // SMTP server runs on 127.0.0.1 in a thread; `build_email_message` + // constructs the message and a PLAINTEXT transport delivers it, and + // the sink records exactly what it received. The production transport + // always uses TLS (A2/A3, gated in CI); that layer is pinned by + // `tls_mode_for_port` and `build_transport_constructs_for_every_port` + // above. This test pins the conversation beneath it: envelope, every + // recipient, and the DATA payload. + + /// A tiny SMTP sink that speaks just enough of the protocol to receive + /// one message and record the envelope + data. Plaintext on purpose + /// (the production transport's TLS is pinned elsewhere). + #[cfg(not(target_arch = "wasm32"))] + mod smtp_sink { + use std::io::{BufRead, BufReader, Write}; + use std::net::TcpListener; + use std::sync::mpsc; + + #[derive(Clone, Debug, Default)] + pub struct Captured { + pub mail_from: Vec, + pub rcpt_to: Vec, + pub data: String, + } + + /// Bind an ephemeral port and serve one SMTP transaction on a + /// background thread. The captured conversation is reported as soon + /// as the DATA payload is complete — NOT on QUIT, because a pooled + /// client may hold the connection open after the send. + pub fn start() -> (u16, mpsc::Receiver) { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind"); + let port = listener.local_addr().expect("addr").port(); + let (tx, rx) = mpsc::channel(); + + std::thread::spawn(move || { + let (stream, _) = listener.accept().expect("accept"); + let mut reader = BufReader::new(stream.try_clone().expect("clone")); + let mut captured = Captured::default(); + + let respond = |code: &str| -> std::io::Result<()> { + let mut w = stream.try_clone().expect("clone"); + w.write_all(code.as_bytes())?; + w.flush() + }; + + respond("220 mock.example ESMTP\r\n").unwrap(); + let mut line = String::new(); + loop { + line.clear(); + if reader.read_line(&mut line).unwrap() == 0 { + break; + } + let cmd = line.trim_end().to_string(); + if cmd.starts_with("EHLO") || cmd.starts_with("HELO") { + respond("250-mock.example\r\n250 8BITMIME\r\n").unwrap(); + } else if cmd.starts_with("MAIL FROM:") { + captured.mail_from.push(cmd); + respond("250 Ok\r\n").unwrap(); + } else if cmd.starts_with("RCPT TO:") { + captured.rcpt_to.push(cmd); + respond("250 Ok\r\n").unwrap(); + } else if cmd == "DATA" { + respond("354 End data with .\r\n").unwrap(); + loop { + line.clear(); + reader.read_line(&mut line).unwrap(); + if line == ".\r\n" || line == ".\n" || line == "." { + break; + } + captured.data.push_str(&line); + } + respond("250 Ok: queued\r\n").unwrap(); + // Report now; do not wait for QUIT/EOF. + let _ = tx.send(captured.clone()); + } else if cmd == "QUIT" { + respond("221 Bye\r\n").unwrap(); + break; + } else { + // RSET, NOOP, or anything unexpected: accept it so + // the conversation can continue. + respond("250 Ok\r\n").unwrap(); + } + } + // If the client never sent DATA (e.g. it aborted), still + // report what little we saw so the test does not hang. + let _ = tx.send(captured); + }); + + (port, rx) + } + } + + /// A message sent through `build_email_message` + a transport reaches + /// the sink with the right envelope, every recipient, and the body. + #[cfg(not(target_arch = "wasm32"))] + #[test] + fn a_real_send_reaches_a_mock_smtp_server() { + use lettre::AsyncSmtpTransport; + + let (port, rx) = smtp_sink::start(); + + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("runtime"); + + rt.block_on(async { + let config = SmtpConfig { + server: "127.0.0.1".into(), + port, + username: "jane@example.com".into(), + password: Secret::new("hunter2"), + from: "jane@example.com".into(), + }; + let email = build_email_message( + &config, + "a@x.com, b@y.com", + "Hello from the test", + "Body text line.", + ) + .expect("message should build"); + + // Plaintext, test-only. The production transport always uses + // TLS (see build_transport); this proves the SMTP conversation + // itself over a real TCP socket. + let mailer = AsyncSmtpTransport::::builder_dangerous("127.0.0.1") + .port(port) + .build(); + mailer.send(email).await.expect("send should succeed"); + }); + + let captured = rx + .recv_timeout(std::time::Duration::from_secs(10)) + .expect("the sink should report the conversation"); + + assert_eq!(captured.mail_from, vec!["MAIL FROM:"]); + assert_eq!( + captured.rcpt_to, + vec!["RCPT TO:", "RCPT TO:"] + ); + // The DATA payload carries the subject header and the body. + assert!( + captured.data.contains("Hello from the test"), + "subject missing from DATA: {:?}", + captured.data + ); + assert!( + captured.data.contains("Body text line."), + "body missing from DATA: {:?}", + captured.data + ); + } + + /// A send to a server that accepts but never speaks (a black hole) + /// fails within the timeout instead of hanging forever — the A6 + /// timeout is wired into the real transport path, not just declared. + #[cfg(not(target_arch = "wasm32"))] + #[test] + fn a_send_to_a_silent_server_errors_instead_of_hanging() { + use lettre::AsyncSmtpTransport; + + // A server that accepts the connection and then says nothing. It + // holds the socket open (blocked on a channel) so the client hangs + // waiting for the greeting unless it times out. + let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind"); + let port = listener.local_addr().expect("addr").port(); + let (release_tx, release_rx) = std::sync::mpsc::channel::<()>(); + let server = std::thread::spawn(move || { + let _stream = listener.accept().expect("accept"); + // Hold the connection open until the test releases us. + let _ = release_rx.recv(); + }); + + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("runtime"); + + let result = rt.block_on(async { + let config = SmtpConfig { + server: "127.0.0.1".into(), + port, + username: "jane@example.com".into(), + password: Secret::new("hunter2"), + from: "jane@example.com".into(), + }; + let email = build_email_message(&config, "a@x.com", "Hi", "Body") + .expect("message should build"); + let mailer = AsyncSmtpTransport::::builder_dangerous("127.0.0.1") + .port(port) + .build(); + // The same bounded path production uses, with a short timeout. + send_bounded(&mailer, email, std::time::Duration::from_secs(1)).await + }); + + // Release the server thread so the process can exit. + let _ = release_tx.send(()); + let _ = server.join(); + + let err = result.expect_err("a silent server must produce an error"); + assert!( + err.contains("timed out"), + "the error should name the timeout, got {err:?}" + ); + } } From 1262e71f9aa3214e4c08542e5ef295b139fe31a3 Mon Sep 17 00:00:00 2001 From: andodeki Date: Mon, 17 Aug 2026 05:08:59 +0000 Subject: [PATCH 3/4] test(uikit): pin the shared conversation click guard (E6) The conversation preview row is consumed by BOTH SMS and email, so a regression there moves two features. Extract should_emit_clicked as a pure function and test it: an empty address suppresses the Clicked action, and so does a scroll in progress. The Clicked payload and props binding are also pinned. nigig-uikit gains its first tests. --- .../conversation/conversation_preview.rs | 84 ++++++++++++++++++- 1 file changed, 80 insertions(+), 4 deletions(-) diff --git a/crates/nigig-uikit/src/shared/conversation/conversation_preview.rs b/crates/nigig-uikit/src/shared/conversation/conversation_preview.rs index c06a0dc..472178e 100644 --- a/crates/nigig-uikit/src/shared/conversation/conversation_preview.rs +++ b/crates/nigig-uikit/src/shared/conversation/conversation_preview.rs @@ -1,6 +1,5 @@ - -use makepad_widgets::*; use makepad_widgets::ActionDefaultRef; +use makepad_widgets::*; script_mod! { use mod.prelude.widgets.* @@ -94,6 +93,18 @@ pub struct SharedConversationPreviewProps { pub was_scrolling: bool, } +/// Should a tap on this row emit a `Clicked` action? +/// +/// Pure so the guard is testable without a `Cx` (E6). A row with an empty +/// address is a placeholder and must never open a thread, and a scroll in +/// progress (a fling) must never be read as a tap. Both SMS and email feed +/// this widget; this function is the single source of truth for the guard, +/// so a regression here surfaces in both features rather than silently in +/// one. +pub fn should_emit_clicked(address: &str, was_scrolling: bool) -> bool { + !address.is_empty() && !was_scrolling +} + #[derive(Clone, Debug, Default)] pub enum SharedConversationPreviewAction { #[default] @@ -134,7 +145,10 @@ impl Widget for SharedConversationPreview { self.view.handle_event(cx, event, scope); - if self.address.is_empty() { + // E6: an empty address (a placeholder row) must never open a + // thread, and a scroll in progress must not be read as a tap. The + // decision is pinned by a unit test below. + if !should_emit_clicked(&self.address, self.was_scrolling) { return; } @@ -142,7 +156,7 @@ impl Widget for SharedConversationPreview { match event.hits(cx, area) { Hit::FingerDown(_fe) => cx.set_key_focus(area), Hit::FingerUp(fe) => { - if fe.was_tap() && fe.is_over && !self.was_scrolling { + if fe.was_tap() && fe.is_over { cx.widget_action( self.widget_uid(), SharedConversationPreviewAction::Clicked { @@ -166,4 +180,66 @@ impl Widget for SharedConversationPreview { } } +#[cfg(test)] +mod tests { + use super::*; + // E6: the conversation kit is consumed by BOTH SMS and email, so a + // regression here moves two features at once. These tests pin the + // click guard and the payload shape, which previously had no coverage. + + /// A row with an address, not mid-scroll, may emit Clicked. + #[test] + fn a_row_with_an_address_may_emit_clicked() { + assert!(should_emit_clicked("alerts@bank.co.ke", false)); + } + + /// The empty-address guard: a placeholder row must never open a thread. + #[test] + fn an_empty_address_suppresses_the_click() { + assert!(!should_emit_clicked("", false)); + } + + /// A fling must not be read as a tap. + #[test] + fn a_scroll_suppresses_the_click() { + assert!(!should_emit_clicked("alerts@bank.co.ke", true)); + assert!(!should_emit_clicked("", true)); + } + + /// The Clicked payload carries exactly the address and display name the + /// row was bound with, so a feature can open the right thread. + #[test] + fn the_clicked_payload_carries_address_and_display_name() { + let payload = SharedConversationPreviewAction::Clicked { + address: "alerts@bank.co.ke".into(), + display_name: "Equity Alerts".into(), + }; + match payload { + SharedConversationPreviewAction::Clicked { + address, + display_name, + } => { + assert_eq!(address, "alerts@bank.co.ke"); + assert_eq!(display_name, "Equity Alerts"); + } + other => panic!("expected Clicked, got {other:?}"), + } + } + + /// The props are the binding between a list row and the emitted action: + /// what a feature puts in (address, display_name) is what a click + /// carries back. Pinned so the two cannot drift apart. + #[test] + fn props_carry_the_address_and_display_name_a_click_will_emit() { + let props = SharedConversationPreviewProps { + address: "jane@example.com".into(), + display_name: "Jane".into(), + was_scrolling: false, + }; + assert_eq!(props.address, "jane@example.com"); + assert_eq!(props.display_name, "Jane"); + assert!(!props.was_scrolling); + assert!(should_emit_clicked(&props.address, props.was_scrolling)); + } +} From fc0b1f287fe216b821549dcb51eddb77f7596b36 Mon Sep 17 00:00:00 2001 From: andodeki Date: Mon, 17 Aug 2026 05:09:05 +0000 Subject: [PATCH 4/4] ci(email): run the conversation-kit tests; mark Phase E complete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit email.yml: the domain test floor ratchets 190 -> 205, and the nigig-email job runs cargo test -p nigig-uikit --lib -- conversation so an email-driven regression in the shared kit cannot silently surface in SMS. The review doc marks E1-E6 done and records the honest correction E5 surfaced: lettre's timeout bounds only the TCP connect, not the greeting/command reads — the send path now bounds the whole operation. --- .forgejo/workflows/email.yml | 13 +++++-- REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md | 41 ++++++++++++++++------ 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/.forgejo/workflows/email.yml b/.forgejo/workflows/email.yml index ba2c7f6..59dd4fe 100644 --- a/.forgejo/workflows/email.yml +++ b/.forgejo/workflows/email.yml @@ -409,11 +409,12 @@ jobs: # A floor, not a ratchet: these tests are cheap, pure, and the # number should only go up. 38 at Phase 0; 154 after C1c/C1d; - # 195 after the Phase C/D completion. + # 195 after the Phase C/D completion; 214 after Phase E (proptest + + # the SMTP sink integration tests). - name: The email domain test suite must not shrink run: | set -euo pipefail - FLOOR=190 + FLOOR=205 out="$(cargo test --locked -p nigig-core --lib -- email_ secret:: mail_backend:: mail_proxy:: imap_client:: credential_store:: 2>&1)" # C1e/C1f: the IMAP transport and the platform keystore are both @@ -482,6 +483,14 @@ jobs: - name: Test run: cargo test --locked -p nigig-email + # E6: the shared conversation kit (nigig-uikit/src/shared/conversation) + # is consumed by BOTH SMS and email, so a regression there moves two + # features at once. It now has its own tests pinning the click guard + # and payload; run them so an email-driven regression cannot silently + # surface in SMS. + - name: Conversation kit tests + run: cargo test --locked -p nigig-uikit --lib -- conversation + # Phase 0.6. A hard gate, not report-only: this crate is 1,100 # lines and already formats clean, so there is no pre-existing # drift to grandfather in. Contrast sms.yml and nigig-map.yml, diff --git a/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md b/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md index 8b70397..f949b4f 100644 --- a/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md +++ b/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md @@ -422,6 +422,21 @@ the unbounded hang I first assumed. But the application sets no timeout of its own, so the user watches `"Testing..."` for a full minute with no way to abort, and a `send` that spans several commands can exceed that. +> **Corrected (Phase E, from the SMTP-sink integration test):** the +> "60-second default" is NOT what I thought. I read lettre 0.11.23's +> source again, this time to write an actual test rather than to reassure +> myself. `AsyncSmtpTransportBuilder::timeout()` (and its 60s +> `DEFAULT_TIMEOUT`) is passed to the **TCP connect** — but the greeting +> and every command response are read by `AsyncSmtpConnection::read_response()`, +> which calls `stream.read_line()` with **no timeout at all**. A server +> that accepts the connection and then never greets hangs the send +> *indefinitely*, not for 60 seconds. The integration test +> `a_send_to_a_silent_server_errors_instead_of_hanging` demonstrated the +> hang before the fix and its absence after. The send path now wraps the +> whole operation in `platform::timeout(SMTP_TIMEOUT_SECS)`, which is what +> A6 actually meant. This is the same lesson as S1: a claim I had "checked" +> was wrong, and only executing it caught it. + Combined with **B4** (no in-flight guard), a user who taps Send during those 60 seconds queues a second delivery. @@ -532,6 +547,7 @@ snapshot. Commits are on `main`. | *(this turn)* | **C1d + C1c DONE** — `mail_proxy.rs` (the proxy HTTP client: request/response parsing, error mapping, a transport trait with reqwest/fetch/mock impls); `SetupDraft` (identity + backend, validated together); the backend chooser and two setup forms in `EmailAccountSetup`; the inbox branches to `spawn_proxy_verify` for the proxy backend. Domain tests **126 → 154**. Coverage tooling (`tools/test-email-coverage.sh`, 93.4% line, floors enforced) wired into `email.yml`. | | *(this turn)* | **Phase C + D COMPLETE** — C1e (`imap_client.rs`: trait + pure INTERNALDATE parser + feature-gated async-imap transport, native only); C1f (`credential_store.rs`: trait + fail-closed default); C3 (`email_cache.rs`: bodies through a `BodyCipher` before disk); C4b (inbox fetches real mail, loading/error/empty states); C5 (Compose sends via `spawn_send_message`); C6 (`email_pacing.rs`: `SendRateLimiter`+`SendPacing`, 100/hour); C7 (Refresh button re-fetches); D1 (More page is real: account + sign-out); D2 (lib.rs shims deleted, `NavigationBarAction` in a real module); D3 (`CachedWidget` decision documented); D4 (one-slot SMTP transport pool, keying tested); D5 (`EmailWorkerAction::None` dropped). Domain tests **154 → 195**. Coverage now **90.7%** over 12 files, floors enforced. | | *(this turn)* | **C6/C7/C1f gaps closed** — C6: `email_bulk.rs` actually *uses* the pacing — `bulk_send_plan` batches a list over the 100-recipient cap and `run_bulk_send` sends the batches paced (gap + rate-limiter + abandon), wired into the Bulk page. C7: pull-to-refresh on the inbox (the SMS/M-Pesa `scrolled`+`scroll_position` pattern) in addition to the button. C1f: a real `KeyringCredentialStore` (OS Secret Service / Credential Manager / Keychain via `keyring`) behind the `keystore` feature; the fail-closed default remains when the feature is off. Domain tests **195 → 206**. | +| *(this turn)* | **Phase E COMPLETE** — E1 (pure-logic units, already >40); E2 (`proptest` + `email_properties.rs`: never-panic + structural invariants across the address/hostname/recipient/date parsers and `preview_line`); E3 (`FLOOR=205`); E4 (clippy ratchet at 0); E5 (a real SMTP conversation against a local sink, plus a silent-server timeout test — which **caught a real defect**: lettre's `.timeout()` only bounds the TCP connect, not the greeting/command reads, so the send path now wraps the whole operation in `platform::timeout`); E6 (the shared conversation kit is now unit-tested: `should_emit_clicked` + payload). Domain tests **206 → 214**; `nigig-uikit` gains its first 5 tests. | **Phase 0 is complete.** All seven items done; 0.7 was fixed upstream. @@ -800,12 +816,12 @@ presenting them as equivalent choices. | ID | Task | |---|---| -| E1 | **Unit-test the pure logic**: port→transport, address parsing/splitting, validation, error mapping. Target ≥40 tests; these need no network. | -| E2 | **Property-test address parsing** (proptest is already a dev-dep in SMS) — never panic on arbitrary input. | -| E3 | **Test-count floor gate**, as SMS has (`FLOOR=100`). | -| E4 | **Clippy ratchet** at the measured baseline. | -| E5 | **Integration test** against a local SMTP sink (e.g. a `MockSmtp` listener on 127.0.0.1) — verifies the transport path without a real provider. | -| **E6** | **NEW — test the shared conversation kit itself.** `nigig-uikit/src/shared/conversation/` is now consumed by both SMS and email, so a change there moves two features and an email-driven regression can surface in SMS. It currently has no tests of its own. At minimum: `SharedConversationPreviewProps` → emitted `Clicked` payload, and that an empty address suppresses the action (the row already guards this, but nothing pins it). | +| ~~E1~~ | ~~**Unit-test the pure logic.**~~ **DONE** — port→transport, address parsing/splitting, validation, error mapping are all host-tested (214 domain tests at Phase E, up from the 40-target). | +| ~~E2~~ | ~~**Property-test address parsing.**~~ **DONE** — `proptest` dev-dependency; `email_properties.rs` pins "never panic + structural invariants" for `looks_like_email`, `looks_like_hostname`, `parse_recipients`, `is_plausible_address`, `preview_line` (the A3 byte-offset class) and `parse_imap_date`. | +| ~~E3~~ | ~~**Test-count floor gate.**~~ **DONE** — `email.yml` enforces `FLOOR=205`. | +| ~~E4~~ | ~~**Clippy ratchet.**~~ **DONE** — ratchet at the measured baseline (0 nigig-email-owned diagnostics). | +| ~~E5~~ | ~~**Integration test against a local SMTP sink.**~~ **DONE** — a hand-rolled SMTP sink on 127.0.0.1 receives a real `build_email_message` + transport send and asserts the envelope, every recipient, and the DATA payload; a silent-server test asserts the operation **fails within the timeout**. This surfaced a real defect: lettre's `.timeout()` only bounds the TCP connect, not the greeting/command reads, so the send path now wraps the whole operation in `platform::timeout` (see the A6/P4 correction below). | +| ~~E6~~ | ~~**Test the shared conversation kit.**~~ **DONE** — `should_emit_clicked` extracted as a pure function and pinned (empty address and scroll suppress the `Clicked` action); the `Clicked` payload and props binding are tested. `nigig-uikit` now has its own tests, run in CI. | --- @@ -827,10 +843,15 @@ convincingly than any client-side fix can. Stated plainly, because the point of this document is to be trusted: -- **No SMTP path has been executed.** No live server, no MITM test. The S1/S3 - analysis is a read of `lettre` 0.11.23's vendored source (`relay()`, - `TlsParameters::new`, `TlsVersion`), which is why I was able to catch my own - error — but it is still a read, not an observed handshake. +- **The SMTP conversation has been executed, but not the TLS handshake.** + Phase E5 runs a real send against a local mock SMTP sink (envelope, every + recipient, and the DATA payload are asserted), and a silent-server test + pins the operation-timeout bound. What is STILL not executed: a TLS + handshake against a real provider, and a MITM test. The S1/S3 analysis of + `relay()`/`TlsParameters::new`/`TlsVersion` remains a read of lettre's + source, not an observed handshake — the sink is plaintext because the + production transport's TLS is pinned separately by `tls_mode_for_port` + and `build_transport_constructs_for_every_port`, not exercised here. - **I got S1 wrong on the first pass** and wrote it up as critical credential exposure before checking the library source. The corrected entry is in §1. Flagging it because a document like this is worth nothing if you cannot tell