From 5702918af132adf4f34b803751e373d9ab2ef7b8 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 15 Oct 2026 21:19:59 +0200 Subject: [PATCH] =?UTF-8?q?config:=20validate=20from=20at=20load=20?= =?UTF-8?q?=E2=80=94=20empty=20or=20malformed=20sender=20addresses=20were?= =?UTF-8?q?=20only=20caught=20at=20send=20time?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- AGENTS.md | 8 +++ CHANGELOG.md | 4 ++ internal/config/config.go | 33 ++++++++++ internal/config/config_test.go | 112 +++++++++++++++++++++++++++++++++ 4 files changed, 157 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index d7ff5fe..4163b22 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -650,6 +650,14 @@ that conversation; "the test was too strict" is not a decision an agent makes al - **Config validation on load** — host:port format, port 1–65535, required fields; `$VAR`/`${VAR}` expansion in `user`/`password`. Tests: `TestValidate*`, `TestExpandEnv`. +- **`from` is validated at load** — every `[[accounts]]` entry and `[[senders]]` alias must + carry a `from` that is a single RFC 5322 address with a non-empty domain + (`validFromAddress`, `internal/config/config.go`, the same bar the send path's + `extractDomain` applies for the Message-ID). Empty or malformed `from` fails + `config.Load` at startup with a clear message — never a blank pre-send `From:` line + that only surfaces as `invalid From address ""` after the mail is written. + Tests: `TestValidate_AccountRequiresFrom`, `TestValidate_SenderRequiresFrom`, + `TestValidate_FromFormat`, `TestLoad_EmptyFromIsLoadError`. - **`password = "keyring"` sentinel** — resolved in `config.Load()` so IMAP, SMTP, and `[[senders]]` aliases all see it; preserved with a warning if the keyring is unavailable. Test: `TestUseKeyring`. diff --git a/CHANGELOG.md b/CHANGELOG.md index 78a23f4..2ff1d5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +# 2026-10-15 + +- **An empty or malformed `from` is rejected at config load, not after the mail is written** — `from = ""` on every account (all-optional defaults) passed validation, showed a blank `From:` line on the pre-send screen, and failed only when the send was attempted: `build message: invalid From address "": cannot parse address for Message-ID`. The send path even documented the assumption — `extractDomain`'s comment says the invalid case "should never happen in practice since the From address always comes from validated config.toml accounts" — a validation that did not exist. `Config.validate()` now requires a `from` on every `[[accounts]]` entry and every `[[senders]]` alias and checks it is a single RFC 5322 address with a non-empty domain (`validFromAddress`, `internal/config/config.go` — the same bar `extractDomain` applies when building the Message-ID, checked at startup instead). Empty gives `account %q: from is required (set from to your address, e.g. "Name ")`; malformed (`from = "xx"`, `Name `, multiple addresses) gives `account %q: from "xx" is not a valid email address`. Both surface through `config.Load`'s existing `config validation:` startup error. Docs' example config already showed `from` filled everywhere; now it is enforced. Tests: `TestValidate_AccountRequiresFrom`, `TestValidate_SenderRequiresFrom`, `TestValidate_FromFormat`, `TestLoad_EmptyFromIsLoadError` (`internal/config/config_test.go`). + # 2026-10-06 - **Long reply chains open in 0.4 s instead of 2.3 s** — a 72 KB back-and-forth thread (Gmail-style `> > > >` quoting, 33 levels deep, 650 quoted lines) took almost 3 s to open. Profiling the stages showed the IMAP fetch and HTML→Markdown conversion at ~50 ms together; glamour's `render.ToANSI` took 2.28 s because its blockquote rendering grows superlinearly with nesting depth (20 identical paragraphs: 9 ms at depth 0, 69 ms at depth 8, 668 ms at depth 33). The reader now caps the *displayed* blockquote depth at 3 (`capQuoteDepth`, `maxReaderQuoteDepth` in `internal/ui/reader.go`) before glamour runs: deeper lines are rewritten to three `>` markers, a blank quoted line is inserted where two capped lines had different original depths so they stay separate paragraphs, and fenced code is left alone. Beyond three `│` bars the levels were unreadable in a terminal anyway. Display only — `Model.openBody` (reply/forward/react quoting, `e` editor view, `O` browser view, `.eml` download) is untouched, and bodies nested ≤ 3 round-trip through `capQuoteDepth` unchanged. Lazy/incremental rendering was considered and rejected: the cost is in nesting depth, not length, so rendering the top first would still have stalled on scroll. Tests: `TestCapQuoteDepth_LimitsNesting`, `TestCapQuoteDepth_ShallowBodyUnchanged`, `TestCapQuoteDepth_DepthChangeKeepsParagraphBreak`, `TestCapQuoteDepth_FencedCodeUntouched`, `TestReader_DeepQuoteChainRendersFast` (`internal/ui/reader_quote_depth_test.go`). diff --git a/internal/config/config.go b/internal/config/config.go index 5ad8da0..f6ab8a9 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -4,6 +4,7 @@ package config import ( "fmt" "net" + "net/mail" "os" "path/filepath" "runtime" @@ -728,6 +729,24 @@ func (cfg *Config) validate() error { if a.User == "" && !a.IsOAuth2() { return fmt.Errorf("account %q: user is required", label) } + if a.From == "" { + return fmt.Errorf("account %q: from is required (set from to your address, e.g. \"Name \")", label) + } + if !validFromAddress(a.From) { + return fmt.Errorf("account %q: from %q is not a valid email address — mail from this account cannot be sent", label, a.From) + } + } + for i, s := range cfg.Senders { + label := s.Name + if label == "" { + label = fmt.Sprintf("senders[%d]", i) + } + if s.From == "" { + return fmt.Errorf("sender %q: from is required — it exists only to send as that address", label) + } + if !validFromAddress(s.From) { + return fmt.Errorf("sender %q: from %q is not a valid email address", label, s.From) + } } // Validate legacy single-account fields if used if cfg.Account.IMAP != "" { @@ -872,6 +891,20 @@ func resolveKeyringPassword(accountName, password string) string { return password // leave sentinel for downstream } +// validFromAddress reports whether s is usable as a sender address: RFC 5322 +// parseable (bare address or "Name ") with a non-empty domain. This is +// the same bar the send path applies when deriving the Message-ID domain +// (extractDomain in internal/smtp), checked here so an unsendable From fails +// at startup instead of after the mail is written. +func validFromAddress(s string) bool { + addr, err := mail.ParseAddress(strings.TrimSpace(s)) + if err != nil { + return false + } + i := strings.LastIndex(addr.Address, "@") + return i >= 0 && i < len(addr.Address)-1 +} + // expandEnv resolves a value that is entirely a single env var reference // ($VAR or ${VAR}). If the value contains other text or multiple $ signs // it is returned as-is, so passwords containing $ are never mangled. diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 9c21451..15018d8 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -362,6 +362,7 @@ func TestValidate_InvalidPort(t *testing.T) { IMAP: "imap.example.com:99999", SMTP: "smtp.example.com:587", User: "test@example.com", + From: "test@example.com", }}, } err := cfg.validate() @@ -380,6 +381,7 @@ func TestValidate_ValidConfig(t *testing.T) { IMAP: "imap.example.com:993", SMTP: "smtp.example.com:587", User: "test@example.com", + From: "test@example.com", }}, } err := cfg.validate() @@ -388,6 +390,115 @@ func TestValidate_ValidConfig(t *testing.T) { } } +// An account without a from must fail at config load — this is the exact +// incident: from = "" on every account loaded fine, showed a blank pre-send +// From line, and failed only after the mail was written. +func TestValidate_AccountRequiresFrom(t *testing.T) { + cfg := &Config{ + Accounts: []AccountConfig{{ + Name: "Main", + IMAP: "imap.example.com:993", + SMTP: "smtp.example.com:587", + User: "me@example.com", + }}, + } + err := cfg.validate() + if err == nil { + t.Fatal("expected error for account with empty from") + } + if !strings.Contains(err.Error(), "from is required") { + t.Errorf("error should mention 'from is required', got: %v", err) + } +} + +// A [[senders]] alias exists only to send as its From address — an empty one +// would put an empty entry in the pre-send ctrl+f cycle. +func TestValidate_SenderRequiresFrom(t *testing.T) { + cfg := &Config{ + Accounts: []AccountConfig{{ + Name: "Test", + IMAP: "imap.example.com:993", + SMTP: "smtp.example.com:587", + User: "test@example.com", + From: "test@example.com", + }}, + Senders: []SenderConfig{{Name: "alias", Account: "Test"}}, + } + err := cfg.validate() + if err == nil { + t.Fatal("expected error for sender with empty from") + } + if !strings.Contains(err.Error(), "from is required") { + t.Errorf("error should mention 'from is required', got: %v", err) + } +} + +// A non-empty from that is not a valid address (from = "xx") must fail at +// load too — the send path's extractDomain applies the same RFC 5322 bar +// when building the Message-ID, so such an account can never send. +func TestValidate_FromFormat(t *testing.T) { + valid := []string{ + "me@example.com", + "Me ", + "me@sub.example.co.uk", + } + invalid := []string{ + "xx", + "Name ", + "xx@", + "@example.com", + "me@example.com, other@example.com", // From is a single address + } + for _, from := range valid { + cfg := &Config{Accounts: []AccountConfig{{ + Name: "T", IMAP: "imap.example.com:993", SMTP: "smtp.example.com:587", + User: "u@example.com", From: from, + }}} + if err := cfg.validate(); err != nil { + t.Errorf("from %q: unexpected error: %v", from, err) + } + } + for _, from := range invalid { + cfg := &Config{Accounts: []AccountConfig{{ + Name: "T", IMAP: "imap.example.com:993", SMTP: "smtp.example.com:587", + User: "u@example.com", From: from, + }}} + err := cfg.validate() + if err == nil { + t.Errorf("from %q: expected validation error, got nil", from) + continue + } + if !strings.Contains(err.Error(), "not a valid email address") { + t.Errorf("from %q: error should mention 'not a valid email address', got: %v", from, err) + } + } +} + +// The from checks run through Load, so a config file with an empty from is +// rejected before neomd starts — not after the first mail is composed. +func TestLoad_EmptyFromIsLoadError(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "config.toml") + cfgBody := ` +[[accounts]] +name = "Main" +imap = "imap.example.com:993" +smtp = "smtp.example.com:587" +user = "me@example.com" +password = "x" +` + if err := os.WriteFile(cfgPath, []byte(cfgBody), 0600); err != nil { + t.Fatalf("write config: %v", err) + } + _, err := Load(cfgPath) + if err == nil { + t.Fatal("expected Load error for empty from") + } + if !strings.Contains(err.Error(), "from is required") { + t.Errorf("error should mention 'from is required', got: %v", err) + } +} + func TestValidate_NegativeUIValues(t *testing.T) { cfg := &Config{ Accounts: []AccountConfig{{ @@ -395,6 +506,7 @@ func TestValidate_NegativeUIValues(t *testing.T) { IMAP: "imap.example.com:993", SMTP: "smtp.example.com:587", User: "test@example.com", + From: "test@example.com", }}, UI: UIConfig{InboxCount: -1}, }