diff --git a/.forgejo/workflows/email.yml b/.forgejo/workflows/email.yml index 08f43d2..6f6ca3f 100644 --- a/.forgejo/workflows/email.yml +++ b/.forgejo/workflows/email.yml @@ -78,6 +78,86 @@ jobs: fi echo "OK" + # A1 / S2. SmtpConfig used to carry `pub password: String` and derive + # Serialize, so on wasm the entire struct -- password included -- was + # serde_json-encoded and POSTed to the email API. Any proxy logging + # request bodies captured the credential. + # + # Two properties must hold, and both are structural rather than + # advisory: + # 1. the password field is a `Secret`, whose Debug renders "***"; + # 2. SmtpConfig does NOT derive Serialize, so it cannot be + # serialised wholesale by accident. The wasm path builds its + # JSON field by field instead, naming the secret at exactly one + # line. + - name: The SMTP password must be a Secret, and the config unserialisable + run: | + set -euo pipefail + bad=0 + decl=$(sed -n '/^pub struct SmtpConfig {/,/^}/p' \ + crates/nigig-core/src/email_worker.rs \ + | grep -E '^[[:space:]]*pub[[:space:]]+password' || true) + if ! echo "$decl" | grep -q 'Secret'; then + echo "ERROR: SmtpConfig.password is not a Secret:" + echo " $decl" + echo "A plain String Debug-prints and serialises the credential." + bad=1 + fi + # The derive list immediately above the struct. + derives=$(grep -B2 '^pub struct SmtpConfig {' \ + crates/nigig-core/src/email_worker.rs | grep '#\[derive' || true) + if echo "$derives" | grep -qE 'Serialize|Deserialize'; then + echo "ERROR: SmtpConfig derives Serialize/Deserialize:" + echo " $derives" + echo "That is how the plaintext password reached the wire. Build" + echo "the request body field by field instead." + bad=1 + fi + [ "$bad" -eq 0 ] || exit 1 + echo "OK" + + # A5. The wasm proxy POST carries the credential, so the endpoint must + # be same-origin or https. The setter used to accept any String, + # including http://, which sends the password in clear text. + - name: The email API endpoint must be validated before use + run: | + set -euo pipefail + if ! grep -q 'fn email_api_url_is_safe' \ + crates/nigig-core/src/email_worker.rs; then + echo "ERROR: email_api_url_is_safe() is gone." + echo "set_email_api_url must reject non-https absolute URLs." + exit 1 + fi + if ! grep -q 'email_api_url_is_safe(&url)' \ + crates/nigig-core/src/email_worker.rs; then + echo "ERROR: set_email_api_url no longer calls the validator." + exit 1 + fi + echo "OK" + + # A3. The TLS mode was implicit -- inherited from lettre's defaults via + # a bare port match. Those defaults are safe, but nothing asserted it, + # so a refactor could have removed encryption with no test failing. + # Every port must map to a TLS mode; there is no cleartext arm. + - name: SMTP transport must never be cleartext + run: | + set -euo pipefail + if ! grep -q 'fn tls_mode_for_port' \ + crates/nigig-core/src/email_worker.rs; then + echo "ERROR: tls_mode_for_port() is gone; the TLS policy is" + echo "implicit again." + exit 1 + fi + if grep -nE 'Tls::None|Tls::Opportunistic' \ + crates/nigig-core/src/email_worker.rs; then + echo + echo "ERROR: a non-mandatory TLS mode above. This transport always" + echo "carries credentials; Opportunistic silently accepts a" + echo "downgrade." + 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. @@ -189,15 +269,15 @@ jobs: echo "$HOME/.cargo/bin" >> "$GITHUB_PATH" - name: Email domain tests - run: cargo test --locked -p nigig-core --lib email_ + run: "cargo test --locked -p nigig-core --lib -- email_ secret::" # A floor, not a ratchet: these tests are cheap, pure, and the # number should only go up. 38 today. - name: The email domain test suite must not shrink run: | set -euo pipefail - FLOOR=38 - out="$(cargo test --locked -p nigig-core --lib email_ 2>&1)" + 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:' \ | sed -n 's/.* \([0-9]\+\) passed.*/\1/p' \ diff --git a/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md b/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md index a3167be..d3853e2 100644 --- a/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md +++ b/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md @@ -528,7 +528,17 @@ snapshot. Commits are on `main`. | `4ae50cb` | **Phase 0.5 DONE** — `serde`, `serde_json`, `robius-location` removed; platform-dep gate extended to `nigig-email`. | | `244c4f2` | **Phase 0.3 + 0.6 DONE** — `email.yml` (4 jobs, 11 gates). Writing the gates caught **B2 and B3 still live in `bulk.rs`**; both fixed here. | +| *(this turn)* | **Phase A complete** — `Secret` newtype; `SmtpConfig` no longer derives `Serialize`; `relay()`; explicit TLS policy; local validation; HTTPS-only proxy endpoint; email THREAT_MODEL. Domain tests **38 → 68**. | + **Phase 0 is complete.** All seven items done; 0.7 was fixed upstream. + +**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 +`Secret` (Debug renders `"***"`, no `Display`, no `Serialize`), the +derive is gone, and the wasm body is built field by field so the +credential appears at exactly one line. Three CI gates enforce it, each +negative-tested. Every gate in `email.yml` was negative-tested — reverted the fix, confirmed the gate fails, restored it — rather than merely observed green. @@ -632,12 +642,12 @@ Exit: `cargo check`/`clippy`/`test -p nigig-email` green on a real runner. | ID | Task | |---|---| -| A1 | **`Secret` newtype for the password.** Redacting `Debug` (`"***"`), `#[serde(skip)]`. **CI gate**: the password field must never be plainly serialisable — mirrors the SMS `#[serde(skip)]` body gate. *This is the real critical item; do it first.* | -| A2 | **Replace the hand-rolled 465 branch with `relay()`.** Behaviour-preserving today (verified equivalent), but stops the code reading as a vulnerability and inherits future hardening. Add a test asserting transport choice per port. | -| A3 | **Pin the TLS policy with a test**, so the inherited defaults cannot be silently removed: assert `Tls::Wrapper`/`Tls::Required` per port and never `Tls::None`. | -| A4 | **Validate before dispatch**: non-empty server/username/from, `from` parses, bounded subject (≤998 bytes per RFC 5322) and body, recipient count cap. All host-testable. | -| A5 | **Scheme-check `set_email_api_url`**; reject non-HTTPS absolute URLs. Document that the wasm endpoint needs auth. | -| A6 | **Add a `THREAT_MODEL.md` section** for email: what the client can enforce, and what it cannot (`from` spoofing is server-side, per SMS E11). | +| ~~A1~~ | **DONE** — `Secret` newtype in `nigig-core/src/secret.rs`. Redacting `Debug` (`"***"`), `#[serde(skip)]`. **CI gate**: the password field must never be plainly serialisable — mirrors the SMS `#[serde(skip)]` body gate. *This is the real critical item; do it first.* | +| ~~A2~~ | **DONE** — now uses `relay()`. Behaviour-preserving today (verified equivalent), but stops the code reading as a vulnerability and inherits future hardening. Add a test asserting transport choice per port. | +| ~~A3~~ | **DONE** — `tls_mode_for_port` + tests., so the inherited defaults cannot be silently removed: assert `Tls::Wrapper`/`Tls::Required` per port and never `Tls::None`. | +| ~~A4~~ | **DONE** — `validate_send` + `config_warning`.: non-empty server/username/from, `from` parses, bounded subject (≤998 bytes per RFC 5322) and body, recipient count cap. All host-testable. | +| ~~A5~~ | **DONE** — `email_api_url_is_safe`.; reject non-HTTPS absolute URLs. Document that the wasm endpoint needs auth. | +| ~~A6~~ | **DONE** — `crates/apps/nigig-email/THREAT_MODEL.md`. for email: what the client can enforce, and what it cannot (`from` spoofing is server-side, per SMS E11). | Negative tests: remove `#[serde(skip)]` → A1 gate fails. Set `Tls::None` on either branch → A3 test fails. diff --git a/crates/apps/nigig-email/THREAT_MODEL.md b/crates/apps/nigig-email/THREAT_MODEL.md new file mode 100644 index 0000000..f857d4f --- /dev/null +++ b/crates/apps/nigig-email/THREAT_MODEL.md @@ -0,0 +1,216 @@ +# nigig-email — Threat Model + +Scope: `crates/apps/nigig-email` and the email modules it depends on in +`crates/nigig-core` (`email_account`, `email_store`, `email_worker`, +`secret`). + +Separate from the root `THREAT_MODEL.md`, which is Nigig-Pay's and shares +no assets with this feature. + +Written as part of Phase A of +`REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md`. Each mitigation names the +control, so a reader can check the claim rather than take it on trust. + +--- + +## 1. Assets + +| # | Asset | Why it matters | +|---|---|---| +| A1 | **The user's mailbox password or app password** | The highest-value secret in the app. For most people email is the password-reset channel for every other account they own, so losing it is worse than losing a payment credential. | +| A2 | Message bodies and subjects | Private correspondence. May contain financial and identity data. | +| A3 | The sender's address and contact graph | Who the user talks to is sensitive even when content is not. | +| A4 | SMTP/IMAP server names and ports | Low value alone; useful for targeting. | +| A5 | The proxy API endpoint and its token (once C1d lands) | A revocable stand-in for A1 — strictly safer, because revoking it does not require a password change. | + +## 2. Trust boundaries + +``` + ┌─ device ───────────────────────────────────┐ + │ UI widgets ──▶ nigig-core (validation) │ + │ │ │ + │ ▼ │ + │ Secret (in memory only) │ + └──────────────────────┼─────────────────────┘ + │ ← boundary 1: the network + ┌───────────────┴────────────────┐ + ▼ ▼ + SMTP relay (native) Proxy API (wasm) + TLS required https required +``` + +Boundary 1 is the one that matters. Everything above it is our code; +everything below is a third party we authenticate to with A1. + +## 3. Threats and mitigations + +### T-E1 — Password captured in transit (CRITICAL) + +*An attacker on the network reads the credential during AUTH.* + +Mitigated: + +- Every port maps to a mandatory TLS mode; `tls_mode_for_port` has **no + cleartext arm**. Port 465 → implicit TLS via `relay()`; everything else + → `starttls_relay`, which uses `Tls::Required` and so aborts **before** + sending AUTH if the upgrade fails. That is what defeats a downgrade. +- Certificate and hostname validation are on (`TlsParameters::new` sets + `accept_invalid_certs: false`, `accept_invalid_hostnames: false`, TLS + 1.2 floor). The `dangerous_*` opt-outs are never called. +- A CI gate fails the build if `Tls::None` or `Tls::Opportunistic` appears, + or if `tls_mode_for_port` is deleted. +- On wasm, `set_email_api_url` rejects anything that is not same-origin or + `https://`, **including protocol-relative `//host/path`**, which is http + on an http page and is easy to mistake for a relative path. + +Residual: we trust the platform root store. A device with an attacker- +installed CA can still intercept. Certificate pinning is not implemented. + +### T-E2 — Password leaked by our own code (CRITICAL) + +*The credential escapes through a log, a crash dump, or a serialised +struct — not through an attack, but through carelessness.* + +This was **live**, not hypothetical. `SmtpConfig` derived `Serialize` over +a plaintext `String`, and on wasm the whole struct was `serde_json`-encoded +and POSTed. Any reverse proxy or APM tool logging request bodies captured +it, and nothing in the code said so. + +Mitigated: + +- `Secret` (`crates/nigig-core/src/secret.rs`): `Debug` always renders + `Secret("***")`, with no verbose mode. `Display` is **not implemented**, + so `format!("{s}")` will not compile either. +- `Serialize`/`Deserialize` are **not implemented** on `Secret` and have + been **removed from `SmtpConfig`**. A struct holding a secret cannot be + serialised wholesale; the compiler stops it. The wasm request body is + now assembled field by field, so `password` appears at exactly **one + line** and "what leaves the device?" is answerable by reading one + function. +- `EmailAccount` — the persistable half — has **no password field at all**. + Two controls: a unit test asserting the serialised form contains neither + the secret nor a field named `password`, and a CI gate on the struct + body in case someone deletes the test. +- `expose()` is the single reader, named to be conspicuous in review. +- The secret is cleared the moment a connection is known to have failed. + +**Residual, and worth stating plainly:** `Secret` does **not** zero its +buffer on drop. Without a `zeroize`-style crate the plaintext can persist +in freed heap memory, and on a device with swap it can reach disk. The +type is a leak-through-code control, not an anti-forensics one. + +### T-E3 — Password persisted in cleartext (HIGH) + +*A "remember my settings" feature writes A1 to a JSON file.* + +Not currently possible: nothing persists the credential, and `Secret` +cannot be serialised. That is presently **by omission** rather than by +design — the safest property of the original code was that it never +implemented storage. + +**Open.** Plan item **C1f** must use the platform keystore. The in-repo +precedent is `robius-sms`'s `SmsScheduleCrypto.java` — AES-256-GCM via +`AndroidKeyStore`, failing closed. Until then, a restart requires +re-entering the password, which is the correct trade. + +### T-E4 — Header or content injection (MEDIUM) + +*`\r\n` in a subject or recipient forges headers (BCC, Reply-To).* + +Partly mitigated: + +- `lettre`'s `Message::builder()` encodes headers, so classic CRLF + injection is handled **by the library**, not by us. +- `validate_send` bounds subject at 998 bytes (RFC 5322) and body at 5 MB, + both before any network call. + +Residual: we do not reject control characters in the subject ourselves. We +rely on `lettre`. If the transport is ever swapped — e.g. for the C1d +proxy, which does **not** go through `lettre` — this becomes unmitigated +and the proxy body builder must sanitise. + +### T-E5 — Sender spoofing (MEDIUM, not fixable here) + +*A user sets `from` to an address they do not control.* + +**Cannot be mitigated client-side.** Whether `from` may differ from the +authenticated identity is the relay's policy (SPF/DKIM/DMARC), enforced +server-side. Same conclusion as SMS **E11**. + +What we can do, and do not yet: warn when `from` differs from `username`, +which is the most common cause of a silent provider rejection. **Open**, +folded into A4 follow-up. + +### T-E6 — Unauthenticated proxy endpoint (MEDIUM) + +*Any script on the page drives the send endpoint using the user's +session.* + +Partly mitigated: the endpoint must be https or same-origin (T-E1). + +**Open.** The POST still carries no auth token, no CSRF token and no +request signing. This is a **C1d requirement**, not an afterthought: the +proxy is only safer than on-device IMAP if the token is revocable and +scoped. + +### T-E7 — Denial of service via a hostile message (MEDIUM) + +*A crafted message body crashes the client on render.* + +Mitigated: + +- `preview_line` slices on `char_indices`, never byte offsets. This is the + SMS **A3** bug class: `&s[..n]` panics when `n` is not a UTF-8 boundary, + and one inbound message containing emoji or non-Latin text took down the + whole SMS list on **every frame** until deleted. Email bodies are more + hostile, not less — arbitrary MIME from anyone who knows the address. +- Tested against emoji, Swahili, Arabic, Japanese and deliberately + misaligned mixed text, which is the case that actually triggers it + (uniform emoji happens to land on a boundary). +- A CI gate rejects new byte-offset slicing in the email text helpers. +- Timestamp formatting is total over `i64`, including `MIN`/`MAX`, because + it runs inside `draw_walk` per visible row. + +### T-E8 — Unbounded network operation (LOW) + +*A black-holed relay hangs the UI.* + +Mitigated: a 20-second SMTP timeout, shorter than `lettre`'s 60s-per- +command default. `validate_send` also refuses obviously incomplete +configs locally, so the common failure costs no round trip. + +Residual: no cancel button, and tasks are fire-and-forget with no retained +`JoinHandle`. Plan item **B6**. + +### T-E9 — Accidental duplicate send (LOW) + +*A double tap sends twice — billed, irreversible, to a human.* + +**Open.** No in-flight guard and no confirmation. `robius-sms` solved this +with `BULK_SEND_IN_FLIGHT` (an `AtomicBool` swap) plus two-tap +confirmation; email has the same irreversibility and neither control. +Plan item **B5**. + +## 4. Open risks, ranked + +| Risk | Plan item | Why it is still open | +|---|---|---| +| No keystore-backed credential storage | C1f | Needs platform work; deliberately blocks "remember me" until then | +| Proxy endpoint unauthenticated | C1d | Must land with the proxy backend, not after | +| `Secret` does not zero on drop | — | Needs a `zeroize` dependency; not yet justified, but do not claim the protection | +| No duplicate-send guard | B5 | Small; next phase | +| No cancel on a running send | B6 | Small; next phase | +| `from` ≠ `username` not warned | A4 follow-up | Cheap, high-value diagnostic | +| No certificate pinning | — | Accepted: platform root store is the norm for mail clients | + +## 5. What has not been tested + +Stated because a threat model that overclaims is worse than none: + +- **No live SMTP server has been contacted.** The TLS analysis is a read of + `lettre` 0.11.23's source, plus unit tests on our own port→mode mapping. + No interception has been attempted. +- **The wasm path has not been built or run.** `call_email_api` is + `#[cfg(target_arch = "wasm32")]`; `email_api_url_is_safe` is host-tested, + but the fetch code around it is not. +- **No IMAP code exists yet**, so T-E1/T-E2 cover the SMTP direction only. diff --git a/crates/apps/nigig-email/src/email_frame/pages/inbox.rs b/crates/apps/nigig-email/src/email_frame/pages/inbox.rs index 6b4c1b9..322aae0 100644 --- a/crates/apps/nigig-email/src/email_frame/pages/inbox.rs +++ b/crates/apps/nigig-email/src/email_frame/pages/inbox.rs @@ -26,6 +26,7 @@ use nigig_core::email_store::{ EmailThreadSummary, }; use nigig_core::email_worker::{spawn_smtp_test, EmailWorkerAction, SmtpConfig}; +use nigig_core::secret::Secret; use nigig_uikit::shared::conversation::conversation_preview::{ SharedConversationPreviewAction, SharedConversationPreviewProps, }; @@ -154,8 +155,10 @@ pub struct EmailInboxPage { /// Session-only. Never written to disk -- see the note in /// `nigig_core::email_account`: `EmailAccount` is the persistable half /// and deliberately has no password field. + /// + /// A1: a `Secret`, so a Debug-print of this widget cannot leak it. #[rust] - password: String, + password: Secret, /// The draft awaiting a connection result, so a failure can report /// against the account the user actually typed. #[rust] @@ -318,6 +321,21 @@ impl EmailInboxPage { self.pending = Some(account.clone()); self.password = password.clone(); + // A4 follow-up: warn on a from/username mismatch before spending a + // round trip. Not an error -- some providers allow send-as aliases + // -- but it is the commonest cause of a silent rejection, so the + // user should see it while they can still fix it. + let probe = SmtpConfig { + server: account.smtp_server.clone(), + port: account.smtp_port, + username: account.username.clone(), + password: password.clone(), + from: account.address.clone(), + }; + if let Some(warning) = nigig_core::email_worker::config_warning(&probe) { + self.setup_error(cx, &warning); + } + // Reuse the existing worker. A successful SMTP handshake with // AUTH is the only credential check available without an IMAP // client, and it is the honest one: it proves the account can diff --git a/crates/nigig-core/src/email_account.rs b/crates/nigig-core/src/email_account.rs index fbcd995..7d7818a 100644 --- a/crates/nigig-core/src/email_account.rs +++ b/crates/nigig-core/src/email_account.rs @@ -21,6 +21,7 @@ //! AES-256-GCM/AndroidKeyStore path is the in-repo precedent) without //! changing anything here. +use crate::secret::Secret; use serde::{Deserialize, Serialize}; /// A configured email account, minus the secret. @@ -133,7 +134,7 @@ impl AccountDraft { /// Returns every error found, not just the first, so the form can /// mark all bad fields in one pass instead of making the user /// resubmit five times. - pub fn validate(&self) -> Result<(EmailAccount, String), Vec> { + pub fn validate(&self) -> Result<(EmailAccount, Secret), Vec> { let mut errors = Vec::new(); let address = self.address.trim(); @@ -198,7 +199,7 @@ impl AccountDraft { username, display_name: self.display_name.trim().to_string(), }, - self.password.clone(), + Secret::new(self.password.clone()), )) } } @@ -301,7 +302,7 @@ mod tests { let (account, secret) = good_draft().validate().expect("should validate"); assert_eq!(account.address, "jane@example.com"); assert_eq!(account.smtp_port, 587); - assert_eq!(secret, "hunter2"); + assert_eq!(secret.expose(), "hunter2"); } /// The persistable half must never carry the secret. This is the @@ -380,7 +381,7 @@ mod tests { ..good_draft() }; let (_, secret) = draft.validate().unwrap(); - assert_eq!(secret, " spaced "); + assert_eq!(secret.expose(), " spaced "); } #[test] diff --git a/crates/nigig-core/src/email_worker.rs b/crates/nigig-core/src/email_worker.rs index 421ecb5..d192d8e 100644 --- a/crates/nigig-core/src/email_worker.rs +++ b/crates/nigig-core/src/email_worker.rs @@ -1,44 +1,189 @@ - use makepad_widgets::*; -use serde::{Deserialize, Serialize}; -#[derive(Clone, Debug, Serialize, Deserialize)] +use crate::secret::Secret; + +/// SMTP connection settings for one send. +/// +/// A1 / assessment finding S2. This type used to derive `Serialize`, +/// `Deserialize` and `Debug` over a plaintext `password: String`. On wasm +/// the whole struct was `serde_json`-encoded and POSTed to the email API, +/// so every request carried the password in clear text -- and any proxy +/// logging request bodies captured it. +/// +/// Two changes, both structural rather than advisory: +/// +/// * the password is a `Secret`, whose `Debug` always renders `"***"`, +/// so printing the config cannot leak it; +/// * `Serialize`/`Deserialize` are GONE. The wasm path now builds its +/// JSON field by field and names the secret at exactly one line, so +/// "does this leave the device?" is answerable by reading one +/// function instead of trusting a derive. +#[derive(Clone, Debug, Default)] pub struct SmtpConfig { pub server: String, pub port: u16, pub username: String, - pub password: String, + pub password: Secret, pub from: String, } -impl Default for SmtpConfig { - fn default() -> Self { - Self { - server: String::new(), - port: 587, - username: String::new(), - password: String::new(), - from: String::new(), - } +impl SmtpConfig { + /// Standard submission port (STARTTLS). `Default` gives port 0, which + /// is never valid, so callers that do not set a port explicitly should + /// use this. + pub const DEFAULT_PORT: u16 = 587; + + /// True when this config cannot possibly authenticate. Cheap guard + /// before spending a network round trip (A4). + pub fn is_incomplete(&self) -> bool { + self.server.trim().is_empty() + || self.username.trim().is_empty() + || self.from.trim().is_empty() + || self.password.is_blank() + || self.port == 0 } } /// Configure the HTTP API endpoint used for email on wasm. No-op on native. +/// +/// A5: rejects anything that is not either a same-origin relative path or +/// an `https://` URL. The old setter took any `String`, so a caller could +/// point the credential-bearing POST at `http://` and have it sent in +/// clear text over the network -- which is the one thing the proxy design +/// is supposed to prevent. +/// +/// Returns whether the URL was accepted, so a caller that ignores the +/// result still cannot silently downgrade: the endpoint simply stays at +/// its safe default. #[cfg(target_arch = "wasm32")] -pub fn set_email_api_url(url: String) { - EMAIL_API_URL.set(url).ok(); +pub fn set_email_api_url(url: String) -> bool { + if !email_api_url_is_safe(&url) { + return false; + } + EMAIL_API_URL.set(url).is_ok() +} + +/// Is this an endpoint we are willing to send credentials to? +/// +/// Split out from the setter so it is testable on the host -- the wasm +/// target cannot run `cargo test` here, and an unvalidated validator is +/// not a control. +pub fn email_api_url_is_safe(url: &str) -> bool { + let u = url.trim(); + if u.is_empty() { + return false; + } + // Relative, same-origin: inherits the page's scheme, which is https + // wherever this is deployed over https. + if u.starts_with('/') && !u.starts_with("//") { + return true; + } + // Absolute: https only. Note `//host/path` is protocol-relative and + // therefore http on an http page, so it is rejected above. + u.starts_with("https://") && u.len() > "https://".len() } /// Spawn an async SMTP test task. Posts `EmailWorkerAction::SmtpTestResult` on completion. +/// +/// A4: refuses an incomplete config locally instead of spending a network +/// round trip to be told the obvious. The old code spawned regardless, so +/// an empty server produced a raw `lettre` DNS error the user could not +/// act on. pub fn spawn_smtp_test(config: SmtpConfig) { + if config.is_incomplete() { + Cx::post_action(EmailWorkerAction::SmtpTestResult(Err( + INCOMPLETE_CONFIG_MESSAGE.to_string(), + ))); + return; + } crate::platform::spawn(async move { let result = smtp_test_impl(&config).await; Cx::post_action(EmailWorkerAction::SmtpTestResult(result)); }); } +/// Shown when a send is attempted without enough settings to try. +pub const INCOMPLETE_CONFIG_MESSAGE: &str = + "Missing account settings. Check the server, port, username, password and from address."; + +/// RFC 5322 caps a header line at 998 octets. A subject longer than that is +/// silently folded or truncated by the provider, so reject it here where we +/// can say why. +pub const MAX_SUBJECT_BYTES: usize = 998; + +/// Upper bound on a single message body. +/// +/// Not a protocol limit -- an application one. Without it a paste of a +/// 50 MB file is attempted verbatim, which stalls the worker and is +/// rejected by every provider anyway. +pub const MAX_BODY_BYTES: usize = 5 * 1024 * 1024; + +/// A non-fatal warning about a config, or None. +/// +/// A4 follow-up / T-E5. Whether `from` may differ from the authenticated +/// identity is the relay's policy, enforced server-side -- we cannot know. +/// But a mismatch is the most common cause of a silent provider rejection, +/// so surfacing it turns "the send failed for no reason" into something +/// actionable. A warning, not an error: some providers legitimately allow +/// send-as aliases. +pub fn config_warning(config: &SmtpConfig) -> Option { + let user = config.username.trim().to_ascii_lowercase(); + let from = config.from.trim().to_ascii_lowercase(); + if user.is_empty() || from.is_empty() || user == from { + return None; + } + // A bare username (no @) is normal for some providers; only warn when + // both look like addresses and they disagree. + if !user.contains('@') { + return None; + } + Some(format!( + "Sending as {from} while signed in as {user}. Some providers reject \ + this unless {from} is a verified alias." + )) +} + +/// Validate the parts of a send that are checkable without a network. +/// +/// A4. Returns the message to show the user, or `Ok(())`. +pub fn validate_send( + config: &SmtpConfig, + to: &str, + subject: &str, + body: &str, +) -> Result<(), String> { + if config.is_incomplete() { + return Err(INCOMPLETE_CONFIG_MESSAGE.to_string()); + } + if to.trim().is_empty() { + return Err("Add at least one recipient.".to_string()); + } + if subject.len() > MAX_SUBJECT_BYTES { + return Err(format!( + "Subject is too long ({} bytes). The limit is {}.", + subject.len(), + MAX_SUBJECT_BYTES + )); + } + if body.len() > MAX_BODY_BYTES { + return Err(format!( + "Message is too large ({} MB). The limit is {} MB.", + body.len() / (1024 * 1024), + MAX_BODY_BYTES / (1024 * 1024) + )); + } + Ok(()) +} + /// 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))); + return; + } crate::platform::spawn(async move { let result = send_email_impl(&config, &to, &subject, &body).await; Cx::post_action(EmailWorkerAction::SendResult(result)); @@ -49,65 +194,124 @@ pub fn spawn_send_email(config: SmtpConfig, to: String, subject: String, body: S #[cfg(not(target_arch = "wasm32"))] use lettre::{ - AsyncSmtpTransport, AsyncTransport, Tokio1Executor, message::{Mailbox, Message}, transport::smtp::authentication::Credentials, + AsyncSmtpTransport, AsyncTransport, Tokio1Executor, }; #[cfg(not(target_arch = "wasm32"))] async fn smtp_test_impl(config: &SmtpConfig) -> Result<(), String> { - let creds = Credentials::new(config.username.clone(), config.password.clone()); + let creds = Credentials::new(config.username.clone(), config.password.expose().to_owned()); let mailer = build_transport(config, creds)?; - mailer.test_connection().await + mailer + .test_connection() + .await .map(|_| ()) .map_err(|e| format!("SMTP test failed: {e}")) } #[cfg(not(target_arch = "wasm32"))] -async fn send_email_impl(config: &SmtpConfig, to: &str, subject: &str, body: &str) -> Result<(), String> { - let from_mbox: Mailbox = config.from.parse() +async fn send_email_impl( + config: &SmtpConfig, + to: &str, + subject: &str, + body: &str, +) -> Result<(), String> { + let from_mbox: Mailbox = config + .from + .parse() .map_err(|e| format!("Invalid from: {e}"))?; - let to_mbox: Mailbox = to.parse() - .map_err(|e| format!("Invalid to: {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) .body(body.to_owned()) .map_err(|e| format!("Build error: {e}"))?; - let creds = Credentials::new(config.username.clone(), config.password.clone()); + let creds = Credentials::new(config.username.clone(), config.password.expose().to_owned()); let mailer = build_transport(config, creds)?; - mailer.send(email).await + mailer + .send(email) + .await .map_err(|e| format!("Send failed: {e}"))?; Ok(()) } +/// Which TLS mode a port implies. +/// +/// A3: the transport policy used to be implicit -- inherited from +/// `lettre`'s defaults via a port `match` with no statement of intent. +/// Those defaults are in fact safe (`TlsParameters::new` sets +/// `accept_invalid_certs: false`, `accept_invalid_hostnames: false` and a +/// TLS 1.2 floor), but nothing in this repository asserted it, so a +/// refactor to `builder_dangerous(..)` without `.tls(..)` would have +/// removed encryption with no test failing. +/// +/// Naming the policy makes it assertable. See `tls_mode_for_port`. +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +pub enum TlsMode { + /// Implicit TLS from the first byte (SMTPS). Port 465. + Implicit, + /// Plain connect, then a mandatory STARTTLS upgrade. Ports 587, 25 and + /// anything else. `lettre`'s `starttls_relay` uses `Tls::Required`, so + /// it aborts before sending AUTH if the upgrade fails -- which is what + /// protects against a downgrade attack. + StartTls, +} + +/// Map a port onto its TLS mode. +/// +/// Pure, so the policy can be unit tested on the host without a server. +/// There is deliberately no third arm: every port gets TLS. A cleartext +/// SMTP mode is not offered, because this transport always carries +/// credentials. +pub fn tls_mode_for_port(port: u16) -> TlsMode { + match port { + IMPLICIT_TLS_PORT => TlsMode::Implicit, + _ => TlsMode::StartTls, + } +} + +/// Implicit-TLS submission port. +pub const IMPLICIT_TLS_PORT: u16 = 465; + #[cfg(not(target_arch = "wasm32"))] fn build_transport( config: &SmtpConfig, creds: Credentials, ) -> Result, String> { - match config.port { - 465 => { - let tls = lettre::transport::smtp::client::TlsParameters::new(config.server.clone()) - .map_err(|e| format!("TLS error: {e}"))?; - Ok(AsyncSmtpTransport::::builder_dangerous(&config.server) - .port(config.port) - .credentials(creds) - .tls(lettre::transport::smtp::client::Tls::Wrapper(tls)) - .build()) - } - _ => { - let builder = AsyncSmtpTransport::::starttls_relay(&config.server) - .map_err(|e| format!("Transport error: {e}"))?; - Ok(builder - .port(config.port) - .credentials(creds) - .build()) - } - } + // A2: use lettre's own `relay()` rather than reassembling it. + // + // The old code called `builder_dangerous(server).tls(Tls::Wrapper(..))` + // by hand for port 465. I checked lettre 0.11.23's source: `relay()` is + // implemented as exactly those calls, so the two were equivalent and + // certificate validation WAS enabled -- I had initially written this up + // as a critical vulnerability and was wrong. + // + // It is still worth replacing, for two reasons that are not + // hypothetical: a reviewer reading `builder_dangerous` reasonably + // assumes the worst (I did), and hand-rolling means this code inherits + // nothing if upstream hardens `relay()` in a later version. + let builder = match tls_mode_for_port(config.port) { + TlsMode::Implicit => AsyncSmtpTransport::::relay(&config.server) + .map_err(|e| format!("TLS transport error: {e}"))?, + TlsMode::StartTls => AsyncSmtpTransport::::starttls_relay(&config.server) + .map_err(|e| format!("Transport error: {e}"))?, + }; + Ok(builder + .port(config.port) + .credentials(creds) + // A6: bound the operation. lettre defaults to 60s per COMMAND, so a + // multi-command send against a black-holed host can hang far longer + // than a user will wait, with the UI stuck on "Sending...". + .timeout(Some(std::time::Duration::from_secs(SMTP_TIMEOUT_SECS))) + .build()) } +/// Per-command SMTP timeout. Shorter than lettre's 60s default: a mobile +/// user on a bad connection needs an error, not a two-minute stall. +pub const SMTP_TIMEOUT_SECS: u64 = 20; + // --- Wasm implementation: HTTP API proxy via fetch --- #[cfg(target_arch = "wasm32")] @@ -118,27 +322,50 @@ static EMAIL_API_URL: OnceLock = OnceLock::new(); #[cfg(target_arch = "wasm32")] fn get_email_api_url() -> &'static str { - EMAIL_API_URL.get().map(|s| s.as_str()).unwrap_or("/api/email") + EMAIL_API_URL + .get() + .map(|s| s.as_str()) + .unwrap_or("/api/email") +} + +/// Build the JSON body for the proxy API. +/// +/// Field by field, deliberately. `SmtpConfig` no longer derives +/// `Serialize`, so this is the ONLY place the password can cross the +/// network, and it is one grep away for anyone auditing what leaves the +/// device. +#[cfg(target_arch = "wasm32")] +fn api_payload(action: &str, config: &SmtpConfig) -> serde_json::Value { + serde_json::json!({ + "action": action, + "config": { + "server": config.server, + "port": config.port, + "username": config.username, + "from": config.from, + // The one line that transmits the secret. + "password": config.password.expose(), + }, + }) } #[cfg(target_arch = "wasm32")] async fn smtp_test_impl(config: &SmtpConfig) -> Result<(), String> { - let body = serde_json::json!({ - "action": "test", - "config": config, - }); + let body = api_payload("test", config); call_email_api(&body.to_string()).await } #[cfg(target_arch = "wasm32")] -async fn send_email_impl(config: &SmtpConfig, to: &str, subject: &str, body: &str) -> Result<(), String> { - let payload = serde_json::json!({ - "action": "send", - "config": config, - "to": to, - "subject": subject, - "body": body, - }); +async fn send_email_impl( + config: &SmtpConfig, + to: &str, + subject: &str, + body: &str, +) -> Result<(), String> { + let mut payload = api_payload("send", config); + payload["to"] = serde_json::Value::from(to); + payload["subject"] = serde_json::Value::from(subject); + payload["body"] = serde_json::Value::from(body); call_email_api(&payload.to_string()).await } @@ -178,12 +405,16 @@ async fn call_email_api(json_body: &str) -> Result<(), String> { } else { let status = resp.status(); let text = JsFuture::from( - resp.text().map_err(|_| "Failed to read response body".to_string())?, + resp.text() + .map_err(|_| "Failed to read response body".to_string())?, ) .await .map_err(|e| format!("Failed to read response: {:?}", e))?; - Err(format!("Email API error ({}): {}", status, - text.as_string().unwrap_or_default())) + Err(format!( + "Email API error ({}): {}", + status, + text.as_string().unwrap_or_default() + )) } } @@ -208,3 +439,236 @@ impl ActionDefaultRef for EmailWorkerAction { &NONE } } + +#[cfg(test)] +mod tests { + use super::*; + + fn good() -> 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(), + } + } + + // ---- A1: the secret must not leak --------------------------------- + + /// The config is Debug-printed in error paths and logs. It must not + /// carry the password out with it. + #[test] + fn debug_printing_the_config_does_not_leak_the_password() { + let rendered = format!("{:?}", good()); + assert!( + rendered.contains("smtp.example.com"), + "should show non-secrets" + ); + assert!(!rendered.contains("hunter2"), "leaked: {rendered}"); + } + + // ---- A3: TLS policy is now asserted, not inherited ---------------- + + #[test] + fn port_465_uses_implicit_tls() { + assert_eq!(tls_mode_for_port(465), TlsMode::Implicit); + assert_eq!(tls_mode_for_port(IMPLICIT_TLS_PORT), TlsMode::Implicit); + } + + /// Everything else upgrades via STARTTLS. There is deliberately no + /// cleartext mode: this transport always carries credentials. + #[test] + fn every_other_port_requires_starttls() { + for port in [25u16, 587, 2525, 8465, 1, 65535] { + assert_eq!( + tls_mode_for_port(port), + TlsMode::StartTls, + "port {port} must still require TLS" + ); + } + } + + #[test] + fn the_timeout_is_shorter_than_lettres_default() { + // lettre defaults to 60s per command; a mobile user needs an error + // sooner than that (A6). + assert!(SMTP_TIMEOUT_SECS < 60); + assert!( + SMTP_TIMEOUT_SECS >= 5, + "not so short that a slow relay fails" + ); + } + + // ---- A4: local validation before spending a round trip ------------ + + #[test] + fn a_complete_config_is_not_incomplete() { + assert!(!good().is_incomplete()); + } + + #[test] + fn each_missing_field_makes_the_config_incomplete() { + let cases: Vec<(&str, SmtpConfig)> = vec![ + ( + "server", + SmtpConfig { + server: " ".into(), + ..good() + }, + ), + ( + "username", + SmtpConfig { + username: String::new(), + ..good() + }, + ), + ( + "from", + SmtpConfig { + from: " ".into(), + ..good() + }, + ), + ( + "password", + SmtpConfig { + password: Secret::new(" "), + ..good() + }, + ), + ("port", SmtpConfig { port: 0, ..good() }), + ]; + for (what, cfg) in cases { + assert!(cfg.is_incomplete(), "missing {what} should be incomplete"); + } + } + + #[test] + fn validate_send_rejects_an_empty_recipient() { + let err = validate_send(&good(), " ", "hi", "body").unwrap_err(); + assert!(err.contains("recipient"), "got: {err}"); + } + + #[test] + fn validate_send_rejects_an_over_long_subject() { + let subject = "a".repeat(MAX_SUBJECT_BYTES + 1); + let err = validate_send(&good(), "a@b.com", &subject, "body").unwrap_err(); + assert!(err.contains("Subject"), "got: {err}"); + } + + #[test] + fn validate_send_accepts_a_subject_at_exactly_the_rfc_limit() { + let subject = "a".repeat(MAX_SUBJECT_BYTES); + assert!(validate_send(&good(), "a@b.com", &subject, "body").is_ok()); + } + + #[test] + fn validate_send_rejects_an_absurdly_large_body() { + let body = "a".repeat(MAX_BODY_BYTES + 1); + let err = validate_send(&good(), "a@b.com", "hi", &body).unwrap_err(); + assert!(err.contains("too large"), "got: {err}"); + } + + #[test] + fn validate_send_accepts_an_ordinary_message() { + assert!(validate_send(&good(), "a@b.com", "Hello", "Body text").is_ok()); + } + + /// The config check must come first: without settings there is nothing + /// to send with, and that is the more actionable message. + #[test] + fn an_incomplete_config_is_reported_before_recipient_problems() { + let cfg = SmtpConfig { + server: String::new(), + ..good() + }; + let err = validate_send(&cfg, "", "", "").unwrap_err(); + assert_eq!(err, INCOMPLETE_CONFIG_MESSAGE); + } + + // ---- A4 follow-up: the from/username mismatch warning ------------- + + #[test] + fn no_warning_when_from_matches_the_username() { + assert!(config_warning(&good()).is_none()); + } + + #[test] + fn warns_when_sending_as_a_different_address() { + let cfg = SmtpConfig { + from: "boss@example.com".into(), + ..good() + }; + let w = config_warning(&cfg).expect("should warn"); + assert!(w.contains("boss@example.com")); + assert!(w.contains("jane@example.com")); + } + + /// Case must not manufacture a warning. + #[test] + fn the_comparison_is_case_insensitive() { + let cfg = SmtpConfig { + from: "JANE@Example.COM".into(), + ..good() + }; + assert!(config_warning(&cfg).is_none()); + } + + /// A bare username is normal for some providers and is not a mismatch. + #[test] + fn a_bare_username_never_warns() { + let cfg = SmtpConfig { + username: "jane".into(), + ..good() + }; + assert!(config_warning(&cfg).is_none()); + } + + #[test] + fn an_empty_field_does_not_warn_that_is_validates_job() { + let cfg = SmtpConfig { + from: String::new(), + ..good() + }; + assert!(config_warning(&cfg).is_none()); + } + + // ---- A5: the proxy endpoint must be https or same-origin ---------- + + #[test] + fn relative_same_origin_paths_are_accepted() { + assert!(email_api_url_is_safe("/api/email")); + assert!(email_api_url_is_safe("/v1/mail/send")); + } + + #[test] + fn https_absolute_urls_are_accepted() { + assert!(email_api_url_is_safe("https://mail.example.com/api")); + } + + /// The whole point: credentials must never go over cleartext http. + #[test] + fn http_and_other_schemes_are_rejected() { + for bad in [ + "http://mail.example.com/api", + "ftp://mail.example.com", + "ws://mail.example.com", + "mail.example.com/api", + "https://", + "", + " ", + ] { + assert!(!email_api_url_is_safe(bad), "should reject {bad:?}"); + } + } + + /// `//host/path` inherits the page scheme, so it is http on an http + /// page. That is exactly the downgrade this check exists to stop, and + /// it is easy to mistake for a relative path. + #[test] + fn protocol_relative_urls_are_rejected() { + assert!(!email_api_url_is_safe("//mail.example.com/api")); + } +} diff --git a/crates/nigig-core/src/lib.rs b/crates/nigig-core/src/lib.rs index dc4cb6f..2d6a55f 100644 --- a/crates/nigig-core/src/lib.rs +++ b/crates/nigig-core/src/lib.rs @@ -19,6 +19,7 @@ pub mod syncing; #[cfg(not(target_arch = "wasm32"))] pub mod tile_service; pub mod email_account; +pub mod secret; pub mod email_store; pub mod email_worker; diff --git a/crates/nigig-core/src/secret.rs b/crates/nigig-core/src/secret.rs new file mode 100644 index 0000000..6f0ecb1 --- /dev/null +++ b/crates/nigig-core/src/secret.rs @@ -0,0 +1,190 @@ +//! A string that does not leak itself. +//! +//! Assessment finding S2: `SmtpConfig` carried `pub password: String` and +//! derived both `Serialize` and `Debug`. Three consequences, all live: +//! +//! 1. On wasm the whole struct is `serde_json`-encoded and POSTed to +//! `/api/email`, so every request carried the plaintext password. Any +//! reverse proxy or APM tool logging request bodies -- which is the +//! default for most -- captured it. +//! 2. `Debug` meant any future `log::debug!("{config:?}")` printed it. +//! Nothing did yet. The type offered no protection, and this is +//! exactly how credentials leak: not deliberately, but because a +//! struct that happened to be printable got printed. +//! 3. `Deserialize` implied an intent to persist. Nothing did yet, which +//! was accidentally the safest property of the whole design. +//! +//! `Secret` fixes the type rather than auditing every call site: +//! +//! * `Debug` renders `Secret("***")` -- always, with no way to opt out. +//! * `Serialize` is NOT implemented. A struct holding a `Secret` cannot +//! be silently serialised; the compiler stops it. This is the point: +//! it is a build failure rather than a code review someone has to +//! remember to do. +//! * `expose()` is the only reader, and it is named to be conspicuous +//! in a diff. +//! +//! Not implemented deliberately: +//! +//! * `Display` -- so `format!("{s}")` cannot print it either. +//! * `Serialize`/`Deserialize` -- see above. Persisting a secret needs +//! the platform keystore (plan A1/C1f), not serde. +//! * Zeroing on drop. That would need a crate like `zeroize`, and +//! honesty matters here: without it the plaintext can persist in freed +//! heap memory. It is a real gap and it is recorded in THREAT_MODEL, +//! rather than pretended away by a type that merely looks careful. + +/// A credential held in memory for the current session. +#[derive(Clone, Default, PartialEq, Eq)] +pub struct Secret(String); + +impl Secret { + pub fn new(value: impl Into) -> Self { + Self(value.into()) + } + + /// Read the plaintext. + /// + /// Named to stand out. Every call is a place the secret enters wider + /// scope, so each should be short-lived and obviously necessary -- + /// handing it to an SMTP `Credentials`, for instance. + pub fn expose(&self) -> &str { + &self.0 + } + + /// A password of only whitespace is a user error, not a credential: + /// some providers accept the AUTH and then fail every send. + pub fn is_blank(&self) -> bool { + self.0.trim().is_empty() + } + + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } + + /// Length, for a strength hint. Does not expose content. + pub fn len(&self) -> usize { + self.0.chars().count() + } + + /// Overwrite with the empty string. + /// + /// Called when a credential is known to be unusable, so a wrong + /// password is not retained for the process lifetime. See the drop + /// caveat in the module docs -- this reduces the window, it does not + /// scrub freed memory. + pub fn clear(&mut self) { + self.0.clear(); + } +} + +/// Always redacted. There is no verbose mode. +impl std::fmt::Debug for Secret { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("Secret(\"***\")") + } +} + +impl From for Secret { + fn from(s: String) -> Self { + Self(s) + } +} + +impl From<&str> for Secret { + fn from(s: &str) -> Self { + Self(s.to_owned()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The whole point of the type. + #[test] + fn debug_never_prints_the_secret() { + let s = Secret::new("hunter2"); + let rendered = format!("{s:?}"); + assert!(!rendered.contains("hunter2"), "leaked: {rendered}"); + assert_eq!(rendered, "Secret(\"***\")"); + } + + /// `{:#?}` takes a different formatter path; it must also redact. + #[test] + fn alternate_debug_also_redacts() { + let s = Secret::new("hunter2"); + assert!(!format!("{s:#?}").contains("hunter2")); + } + + /// A secret nested in a derived-Debug struct is the realistic leak: + /// nobody prints the password directly, they print the config. + #[test] + fn a_struct_containing_a_secret_does_not_leak_it_via_derived_debug() { + #[derive(Debug)] + struct Config { + user: String, + password: Secret, + } + let c = Config { + user: "jane".into(), + password: Secret::new("hunter2"), + }; + let rendered = format!("{c:?}"); + assert!(rendered.contains("jane"), "should still show non-secrets"); + assert!(!rendered.contains("hunter2"), "leaked: {rendered}"); + } + + #[test] + fn expose_returns_the_plaintext_for_the_one_legitimate_use() { + assert_eq!(Secret::new("hunter2").expose(), "hunter2"); + } + + #[test] + fn clear_overwrites_the_value() { + let mut s = Secret::new("hunter2"); + s.clear(); + assert!(s.is_empty()); + assert_eq!(s.expose(), ""); + } + + /// Whitespace-only is blank but not empty: some providers accept the + /// AUTH and then fail every send, which is worse than a clear error. + #[test] + fn blank_and_empty_are_different_questions() { + let spaces = Secret::new(" "); + assert!(spaces.is_blank()); + assert!(!spaces.is_empty()); + + let real = Secret::new("hunter2"); + assert!(!real.is_blank()); + assert!(!real.is_empty()); + } + + /// A password is not trimmed anywhere, so length must count what the + /// user actually typed -- and count chars, not bytes, or a passphrase + /// with non-Latin characters reports the wrong strength. + #[test] + fn len_counts_characters_not_bytes() { + assert_eq!(Secret::new("hunter2").len(), 7); + assert_eq!(Secret::new(" pad ").len(), 7); + assert_eq!(Secret::new("pässwörd").len(), 8); + assert_eq!(Secret::new("🔑🔑").len(), 2); + } + + #[test] + fn converts_from_the_string_types_a_text_input_produces() { + assert_eq!(Secret::from("a".to_string()).expose(), "a"); + assert_eq!(Secret::from("b").expose(), "b"); + assert_eq!(Secret::default().expose(), ""); + } + + /// Equality is needed for tests and for "did the password change?" + /// checks. It must compare content, not the redacted rendering, or + /// every secret would compare equal to every other. + #[test] + fn equality_compares_content_not_the_redaction() { + assert_eq!(Secret::new("a"), Secret::new("a")); + assert_ne!(Secret::new("a"), Secret::new("b")); + } +}