fix: fail honestly on unreadable TLS modes and mis-sized mail #425

2026-08-15T01:25:14Z

KofTwentyTwo commented

2026-08-15 01:25:14 +00:00

(Migrated from github.com)

Two silently-wrong paths: in both, the code did something plausible instead of failing honestly, so the user got a confident wrong answer with no signal at all.

Closes #271
Closes #256

`tls_mode_from_db` silently coerced unreadable TLS modes (#271)

Was: tls_mode_from_db matched "start_tls" and "plain" and sent everything else — including "implicit_tls" itself and any corrupted or tampered token — down a _ => arm to ImplicitTls. This is the path every ordinary account read takes (list_accounts, and get_account through it), not a recovery path. An account configured for STARTTLS whose stored value failed to persist correctly, or was tampered with, came back as implicit TLS: a different transport than the operator asked for, with no error, no warning, and nothing in the logs. A silent change to an account's security posture is the one thing this layer must never do quietly.

Now: the parse fails closed. All three valid tokens are matched explicitly and anything else returns DatabaseError::Corrupted naming the account id, the column, and the offending raw value. list_accounts already returned Result, so no caller signature changed and no caller panics — the failure propagates through the same error path callers already handle.

Both imap_tls_mode and smtp_tls_mode went through this one function, so both are covered by the single fix; the new test exercises each column independently.

This is deliberately stricter than the best-effort salvage path in recovery.rs (fixed in #248), which WARN-logs and falls back. That asymmetry is intentional and documented in the code: salvage exists to rescue what it can from an already-broken file and has no caller to return an error to, whereas this is the normal read path, which does.

Filter `size` was wrong for HTML-only mail and ignored attachments (#256)

Was: size was computed from body_plain alone. An HTML-only message — extremely common — has no plaintext body, so it measured zero bytes and fell on the wrong side of every comparison: WHERE size > 100 said no for a 400-byte message, size < 100 said yes. Attachments were not counted at all, so a two-line note carrying a 5 MB PDF measured about 13 bytes. Size rules are usually written to catch large mail; the field was silently wrong on exactly that mail.

Definition chosen: size is the total byte size of the message content the store holds — all body parts plus all attachment payloads.

Rationale for that choice over the alternatives:

Full RFC822 octets would be the ideal meaning, but the raw octets are not retained and no stored column carries their length (content_hash is a digest over them, not a size). Implementing it would need a schema change to persist a size column — out of scope for a focused fix, and this repo does not migrate databases.
Plaintext length is what was there, and is the bug.
Both body parts summed rather than one preferred: a multipart/alternative message genuinely carries the plaintext and the HTML on the wire, so summing tracks the real message instead of one arbitrary view of it.

Headers and MIME framing are excluded, and the doc comment on FilterField::Size and on the helper says so explicitly. The result is a consistent lower bound on wire size rather than a guess — for base64-encoded attachments the transmitted form is roughly a third larger again. That is a scoped, documented meaning rather than a silently wrong number.

Tests

New tests, each failing without its fix:

unrecognized_stored_tls_mode_fails_the_read_instead_of_defaulting — saves a STARTTLS account, overwrites the stored mode out-of-band with a token this build does not know (writes are validated, so it can only arrive via corruption/tampering), asserts list_accounts returns Corrupted naming account, column, and value, and that get_account fails closed too. Runs for both the IMAP and the SMTP column. Previously returned Ok with a fabricated mode.
size_counts_html_body_of_html_only_message — a 407-byte HTML-only message must satisfy size > 100 and not size < 100. Previously measured 0.
size_counts_attachment_payloads — a 13-byte note with a 5000-byte attachment must satisfy size > 4096. Previously measured 13.
size_sums_both_body_parts_and_attachments — pins the documented definition at an exact 1000 bytes.
size_of_empty_message_is_zero — a genuinely empty message measures zero and still compares as a number.

No existing assertion was weakened or relaxed. Neither behaviour had prior test coverage encoding it: there were no tests for an unrecognized stored TLS mode, and no tests for the size field at all. The existing account_tls_modes_survive_save_and_reload still passes unchanged.

Gates

cargo fmt --all -- --check, cargo check-all, and cargo test-all all clean, plus three RUST_TEST_THREADS=2 cargo test-all runs with zero failures.

Follow-up noted, not fixed here

The same silent-fallback shape exists a few lines away in the same row mapper: serde_json::from_str(&protocol_str).unwrap_or(nuncio_core::AccountProtocol::ImapSmtp) coerces an unparseable stored protocol discriminator to ImapSmtp, which would then read the wrong columns for the row. Left alone to keep this PR scoped to the two issues; worth its own issue.

Two silently-wrong paths: in both, the code did something plausible instead of failing honestly, so the user got a confident wrong answer with no signal at all. Closes #271 Closes #256 ## `tls_mode_from_db` silently coerced unreadable TLS modes (#271) **Was:** `tls_mode_from_db` matched `"start_tls"` and `"plain"` and sent *everything else* — including `"implicit_tls"` itself and any corrupted or tampered token — down a `_ =>` arm to `ImplicitTls`. This is the path every ordinary account read takes (`list_accounts`, and `get_account` through it), not a recovery path. An account configured for STARTTLS whose stored value failed to persist correctly, or was tampered with, came back as implicit TLS: a different transport than the operator asked for, with no error, no warning, and nothing in the logs. A silent change to an account's security posture is the one thing this layer must never do quietly. **Now:** the parse fails closed. All three valid tokens are matched explicitly and anything else returns `DatabaseError::Corrupted` naming the account id, the column, and the offending raw value. `list_accounts` already returned `Result`, so no caller signature changed and no caller panics — the failure propagates through the same error path callers already handle. Both `imap_tls_mode` and `smtp_tls_mode` went through this one function, so both are covered by the single fix; the new test exercises each column independently. This is deliberately stricter than the best-effort salvage path in `recovery.rs` (fixed in #248), which WARN-logs and falls back. That asymmetry is intentional and documented in the code: salvage exists to rescue what it can from an already-broken file and has no caller to return an error to, whereas this is the normal read path, which does. ## Filter `size` was wrong for HTML-only mail and ignored attachments (#256) **Was:** `size` was computed from `body_plain` alone. An HTML-only message — extremely common — has no plaintext body, so it measured **zero bytes** and fell on the wrong side of every comparison: `WHERE size > 100` said no for a 400-byte message, `size < 100` said yes. Attachments were not counted at all, so a two-line note carrying a 5 MB PDF measured about 13 bytes. Size rules are usually written to catch large mail; the field was silently wrong on exactly that mail. **Definition chosen:** `size` is the **total byte size of the message content the store holds — all body parts plus all attachment payloads.** Rationale for that choice over the alternatives: - *Full RFC822 octets* would be the ideal meaning, but the raw octets are not retained and no stored column carries their length (`content_hash` is a digest over them, not a size). Implementing it would need a schema change to persist a size column — out of scope for a focused fix, and this repo does not migrate databases. - *Plaintext length* is what was there, and is the bug. - *Both body parts summed* rather than one preferred: a `multipart/alternative` message genuinely carries the plaintext **and** the HTML on the wire, so summing tracks the real message instead of one arbitrary view of it. Headers and MIME framing are excluded, and the doc comment on `FilterField::Size` and on the helper says so explicitly. The result is a consistent lower bound on wire size rather than a guess — for base64-encoded attachments the transmitted form is roughly a third larger again. That is a scoped, documented meaning rather than a silently wrong number. ## Tests New tests, each failing without its fix: - `unrecognized_stored_tls_mode_fails_the_read_instead_of_defaulting` — saves a STARTTLS account, overwrites the stored mode out-of-band with a token this build does not know (writes are validated, so it can only arrive via corruption/tampering), asserts `list_accounts` returns `Corrupted` naming account, column, and value, and that `get_account` fails closed too. Runs for both the IMAP and the SMTP column. Previously returned `Ok` with a fabricated mode. - `size_counts_html_body_of_html_only_message` — a 407-byte HTML-only message must satisfy `size > 100` and not `size < 100`. Previously measured 0. - `size_counts_attachment_payloads` — a 13-byte note with a 5000-byte attachment must satisfy `size > 4096`. Previously measured 13. - `size_sums_both_body_parts_and_attachments` — pins the documented definition at an exact 1000 bytes. - `size_of_empty_message_is_zero` — a genuinely empty message measures zero and still compares as a number. **No existing assertion was weakened or relaxed.** Neither behaviour had prior test coverage encoding it: there were no tests for an unrecognized stored TLS mode, and no tests for the `size` field at all. The existing `account_tls_modes_survive_save_and_reload` still passes unchanged. ## Gates `cargo fmt --all -- --check`, `cargo check-all`, and `cargo test-all` all clean, plus three `RUST_TEST_THREADS=2 cargo test-all` runs with zero failures. ## Follow-up noted, not fixed here The same silent-fallback shape exists a few lines away in the same row mapper: `serde_json::from_str(&protocol_str).unwrap_or(nuncio_core::AccountProtocol::ImapSmtp)` coerces an unparseable stored `protocol` discriminator to `ImapSmtp`, which would then read the wrong columns for the row. Left alone to keep this PR scoped to the two issues; worth its own issue.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#425