andodeki/nigig-org
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 8

Compare commits

base: andodeki:3786e7c1cfff967a4c195410f84f44bc58507e4d
Branches Tags
andodeki:main
..
compare: andodeki:58b0bac062cdd381b51c77e6c861723f3445ec97
Branches Tags
andodeki:main

No commits in common. "3786e7c1cfff967a4c195410f84f44bc58507e4d" and "58b0bac062cdd381b51c77e6c861723f3445ec97" have entirely different histories.
3786e7c1cf ... 58b0bac062

8 changed files with 73 additions and 1053 deletions
Download patch file Download diff file Expand all files Collapse all files

86
.forgejo/workflows/email.yml
View file

@ -78,86 +78,6 @@ 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.
@ -269,15 +189,15 @@ jobs:
echo "$HOME/.cargo/bin" >> "$GITHUB_PATH"
- name: Email domain tests
run: "cargo test --locked -p nigig-core --lib -- email_ secret::"
run: cargo test --locked -p nigig-core --lib email_
# 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=60
out="$(cargo test --locked -p nigig-core --lib -- email_ secret:: 2>&1)"
FLOOR=38
out="$(cargo test --locked -p nigig-core --lib email_ 2>&1)"
echo "$out" | grep -E '^test result:' || true
n=$(echo "$out" | grep -E '^test result:' \
| sed -n 's/.* \([0-9]\+\) passed.*/\1/p' \

22
REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md
View file

@ -528,17 +528,7 @@ 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.
@ -642,12 +632,12 @@ Exit: `cargo check`/`clippy`/`test -p nigig-email` green on a real runner.
| ID | Task |
|---|---|
| ~~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). |
| A1 | **`Secret<String>` 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). |
Negative tests: remove `#[serde(skip)]` → A1 gate fails. Set
`Tls::None` on either branch → A3 test fails.

216
crates/apps/nigig-email/THREAT_MODEL.md
View file

@ -1,216 +0,0 @@
# 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.

20
crates/apps/nigig-email/src/email_frame/pages/inbox.rs
View file

@ -26,7 +26,6 @@ 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,
};
@ -155,10 +154,8 @@ 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: Secret,
password: String,
/// The draft awaiting a connection result, so a failure can report
/// against the account the user actually typed.
#[rust]
@ -321,21 +318,6 @@ 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

9
crates/nigig-core/src/email_account.rs
View file

@ -21,7 +21,6 @@
//! 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.
@ -134,7 +133,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, Secret), Vec<AccountError>> {
pub fn validate(&self) -> Result<(EmailAccount, String), Vec<AccountError>> {
let mut errors = Vec::new();
let address = self.address.trim();
@ -199,7 +198,7 @@ impl AccountDraft {
username,
display_name: self.display_name.trim().to_string(),
},
Secret::new(self.password.clone()),
self.password.clone(),
))
}
}
@ -302,7 +301,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.expose(), "hunter2");
assert_eq!(secret, "hunter2");
}
/// The persistable half must never carry the secret. This is the
@ -381,7 +380,7 @@ mod tests {
..good_draft()
};
let (_, secret) = draft.validate().unwrap();
assert_eq!(secret.expose(), " spaced ");
assert_eq!(secret, " spaced ");
}
#[test]

582
crates/nigig-core/src/email_worker.rs
View file

@ -1,189 +1,44 @@
use makepad_widgets::*;
use serde::{Deserialize, Serialize};
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)]
#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct SmtpConfig {
pub server: String,
pub port: u16,
pub username: String,
pub password: Secret,
pub password: String,
pub from: String,
}
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
impl Default for SmtpConfig {
fn default() -> Self {
Self {
server: String::new(),
port: 587,
username: String::new(),
password: String::new(),
from: String::new(),
}
}
}
/// 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) -> 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()
pub fn set_email_api_url(url: String) {
EMAIL_API_URL.set(url).ok();
}
/// 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<String> {
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));
@ -194,124 +49,65 @@ 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.expose().to_owned());
let creds = Credentials::new(config.username.clone(), config.password.clone());
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.expose().to_owned());
let creds = Credentials::new(config.username.clone(), config.password.clone());
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<AsyncSmtpTransport<Tokio1Executor>, String> {
// 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::<Tokio1Executor>::relay(&config.server)
.map_err(|e| format!("TLS transport error: {e}"))?,
TlsMode::StartTls => AsyncSmtpTransport::<Tokio1Executor>::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())
match config.port {
465 => {
let tls = lettre::transport::smtp::client::TlsParameters::new(config.server.clone())
.map_err(|e| format!("TLS error: {e}"))?;
Ok(AsyncSmtpTransport::<Tokio1Executor>::builder_dangerous(&config.server)
.port(config.port)
.credentials(creds)
.tls(lettre::transport::smtp::client::Tls::Wrapper(tls))
.build())
}
_ => {
let builder = AsyncSmtpTransport::<Tokio1Executor>::starttls_relay(&config.server)
.map_err(|e| format!("Transport error: {e}"))?;
Ok(builder
.port(config.port)
.credentials(creds)
.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")]
@ -322,50 +118,27 @@ static EMAIL_API_URL: OnceLock<String> = 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")
}
/// 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(),
},
})
EMAIL_API_URL.get().map(|s| s.as_str()).unwrap_or("/api/email")
}
#[cfg(target_arch = "wasm32")]
async fn smtp_test_impl(config: &SmtpConfig) -> Result<(), String> {
let body = api_payload("test", config);
let body = serde_json::json!({
"action": "test",
"config": 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 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);
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,
});
call_email_api(&payload.to_string()).await
}
@ -405,16 +178,12 @@ 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()))
}
}
@ -439,236 +208,3 @@ 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"));
}
}

1
crates/nigig-core/src/lib.rs
View file

@ -19,7 +19,6 @@ 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;

190
crates/nigig-core/src/secret.rs
View file

@ -1,190 +0,0 @@
//! 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<String>) -> 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<String> 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"));
}
}