fix(config): reject garbage numerics instead of silently reading zero, item 123

toInt() and toLongLong() return 0 on failure rather than the default, so a
typo in autosave_interval_ms produced a zero-interval timer. That timer is
restarted on every keystroke, so it would fire on the next event-loop pass
and turn a 30 second debounce into a Maildir write per keystroke, each one
uploaded by mbsync: exactly the behaviour the debounce exists to prevent.

This file already had the right shape in five places, a checked parse that
reports the bad value and keeps the default. The [compose] keys were the
only numerics skipping it. The interval is also clamped, since nothing
assigns a meaning to a zero or negative autosave.

quote_position now warns on an unrecognised value, matching sync_on_exit,
language and date_format; the only silent fallbacks in this file are for
absent keys rather than malformed ones. And a missing `sent` folder is a
notice rather than a problem, because the spec blesses that configuration
and a modal on every launch for a permanently correct setup is how users
learn to dismiss dialogs unread.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015muoUo2GdxmBDSp5vjYcbE

4 files changed, 201 insertions, 17 deletions

diff --git a/src/config.cpp b/src/config.cpp
index 09869b9..c6bedd2 100644
--- a/src/config.cpp
+++ b/src/config.cpp
@@ -525,26 +525,90 @@ void Config::load(const QString &path)
     }
 
     settings.beginGroup(QStringLiteral("compose"));
-    // value(key, default) throughout rather than testing contains(): an
-    // absent key and a key set to its default must behave identically, and
-    // send_delay_ms = 0 is a REAL setting meaning "send at once" that a
-    // zero-test would mistake for unset.
-    m_compose.quotePosition =
+    // Absent keys stay silent (the struct's own default holds), but a
+    // PRESENT and malformed value is reported: value(key, default) alone
+    // would happily accept "quote_position = abov" as Above, matching every
+    // other enum-ish key in this file (sync_on_exit, language, date_format)
+    // rather than being the one silent exception.
+    const QString quotePosition =
         settings.value(QStringLiteral("quote_position"), QStringLiteral("above"))
-                    .toString().compare(QStringLiteral("below"), Qt::CaseInsensitive) == 0
-            ? ComposeSettings::QuotePosition::Below
-            : ComposeSettings::QuotePosition::Above;
+            .toString().trimmed();
+    if (quotePosition.compare(QStringLiteral("above"), Qt::CaseInsensitive) == 0) {
+        m_compose.quotePosition = ComposeSettings::QuotePosition::Above;
+    } else if (quotePosition.compare(QStringLiteral("below"), Qt::CaseInsensitive) == 0) {
+        m_compose.quotePosition = ComposeSettings::QuotePosition::Below;
+    } else {
+        addProblem(tr("[compose] quote_position '%1' is not recognised; "
+                      "expected above or below. Using above.")
+                       .arg(quotePosition));
+    }
+
     m_compose.sendHtml =
         settings.value(QStringLiteral("send_html"), true).toBool();
-    m_compose.autosaveIntervalMs =
-        settings.value(QStringLiteral("autosave_interval_ms"), 30000).toInt();
-    m_compose.sendDelayMs =
-        settings.value(QStringLiteral("send_delay_ms"), 5000).toInt();
+
+    // Three numerics, all following the shape already established at
+    // message_zoom, toolbar_icon_size, mark_read_delay_ms and
+    // auto_sync_delay_ms elsewhere in this function: a QVariant, a checked
+    // toInt()/toLongLong(), and a reported fallback to the struct's own
+    // default on failure. The bare toInt()/toLongLong() this replaced return
+    // 0 on a PARSE FAILURE, not the default, which is silently indistinguishable
+    // from the user writing 0 on purpose. For autosave_interval_ms that 0
+    // reaches a QTimer restarted on every keystroke, so it would fire on the
+    // very next event-loop pass and turn the debounce into a write per
+    // keystroke, each one uploaded by mbsync.
+    const QVariant autosave = settings.value(QStringLiteral("autosave_interval_ms"));
+    if (autosave.isValid()) {
+        bool ok = false;
+        const int value = autosave.toString().trimmed().toInt(&ok);
+        if (ok) {
+            // Clamped, not merely parsed: nothing in the spec assigns a
+            // meaning to a zero or negative autosave interval, unlike
+            // mark_read_delay_ms where negative-means-off is documented
+            // behaviour. A zero interval here is the same runaway-write
+            // hazard as the parse failure above, just spelled correctly.
+            m_compose.autosaveIntervalMs = qMax(1000, value);
+        } else {
+            addProblem(tr("[compose] autosave_interval_ms '%1' is not a "
+                          "number; using %2.")
+                           .arg(autosave.toString())
+                           .arg(m_compose.autosaveIntervalMs));
+        }
+    }
+
+    // Zero is a REAL setting here, meaning "send at once", and must be
+    // honoured rather than mistaken for unset: that is exactly why this is
+    // isValid()-then-checked-parse rather than a zero-test.
+    const QVariant sendDelay = settings.value(QStringLiteral("send_delay_ms"));
+    if (sendDelay.isValid()) {
+        bool ok = false;
+        const int value = sendDelay.toString().trimmed().toInt(&ok);
+        if (ok) {
+            m_compose.sendDelayMs = value;
+        } else {
+            addProblem(tr("[compose] send_delay_ms '%1' is not a number; "
+                          "using %2.")
+                           .arg(sendDelay.toString())
+                           .arg(m_compose.sendDelayMs));
+        }
+    }
+
     m_compose.defaultAccount =
         settings.value(QStringLiteral("default_account")).toString().trimmed();
-    m_compose.attachmentWarnBytes =
-        settings.value(QStringLiteral("attachment_warn_bytes"), qint64(26214400))
-            .toLongLong();
+
+    const QVariant attachmentWarn =
+        settings.value(QStringLiteral("attachment_warn_bytes"));
+    if (attachmentWarn.isValid()) {
+        bool ok = false;
+        const qint64 value = attachmentWarn.toString().trimmed().toLongLong(&ok);
+        if (ok) {
+            m_compose.attachmentWarnBytes = value;
+        } else {
+            addProblem(tr("[compose] attachment_warn_bytes '%1' is not a "
+                          "number; using %2.")
+                           .arg(attachmentWarn.toString())
+                           .arg(m_compose.attachmentWarnBytes));
+        }
+    }
     settings.endGroup();
 
     loadSavedQueries(path, settings);
@@ -570,6 +634,11 @@ void Config::load(const QString &path)
     // and expects mail to come from it, unlike an installation where no
     // account can send at all, which is a valid read-only setup and not
     // warned about below.
+    //
+    // Unlike startup_account just above, the bad value is NOT cleared after
+    // the warning: the composer resolves this through canSend() at the point
+    // of use, so a value naming an unusable account is simply skipped there
+    // rather than needing to be blanked here.
     if (!m_compose.defaultAccount.isEmpty()) {
         const auto named = std::find_if(
             m_accounts.cbegin(), m_accounts.cend(),
@@ -593,12 +662,20 @@ void Config::load(const QString &path)
     for (const Account &account : m_accounts) {
         if (!account.canSend())
             continue;
+        // A notice, not a problem: a provider whose SMTP server files sent
+        // mail on its own is a legitimate, permanently correct configuration.
+        // addProblem() here would raise a startup modal on every launch for a
+        // setup that will never change, which is exactly how a user learns to
+        // dismiss dialogs unread.
         if (account.sent.isEmpty()) {
-            addProblem(
+            addNotice(
                 tr("Account '%1' can send but configures no `sent` folder, so "
                    "no local copy of sent mail is filed.")
                     .arg(account.key));
         }
+        // Still a problem: unlike a missing sent folder, this is a real loss
+        // of protection (no draft is saved while composing) rather than a
+        // deliberate provider-side choice.
         if (account.drafts.isEmpty()) {
             addProblem(
                 tr("Account '%1' can send but configures no `drafts` folder, "
diff --git a/src/types.h b/src/types.h
index 0c6532e..99c271d 100644
--- a/src/types.h
+++ b/src/types.h
@@ -249,7 +249,7 @@ struct ComposeContext
 {
     enum class Kind { New, Reply, ReplyAll, Forward };
 
-    QString accountKey;          ///< Which account sends. Resolved by ComposeContext's rules.
+    QString accountKey;          ///< Which account sends. Plain data here; the resolution rules live with whatever builds this context.
     Kind kind = Kind::New;
     QString originalPath;        ///< The .eml being replied to or forwarded. Empty for New.
     QString inReplyTo;           ///< Message-ID of the original.
diff --git a/tests/test_config.cpp b/tests/test_config.cpp
index 2df4c40..a902425 100644
--- a/tests/test_config.cpp
+++ b/tests/test_config.cpp
@@ -126,6 +126,11 @@ private slots:
     void aZeroSendDelayIsHonouredRatherThanTreatedAsUnset();
     void aDefaultAccountThatCannotSendIsWarnedAbout();
     void anInstallationWhereNoAccountCanSendIsNotWarnedAbout();
+    void garbageAutosaveIntervalIsRejectedNotZero();
+    void garbageSendDelayIsRejectedNotZero();
+    void garbageAttachmentWarnBytesIsRejectedNotZero();
+    void zeroOrNegativeAutosaveIntervalIsClamped();
+    void unrecognisedQuotePositionWarnsAndFallsBackToAbove();
 };
 
 static QString writeIni(const QTemporaryDir &dir, const QString &body)
@@ -2418,5 +2423,91 @@ void TestConfig::anInstallationWhereNoAccountCanSendIsNotWarnedAbout()
     }
 }
 
+void TestConfig::garbageAutosaveIntervalIsRejectedNotZero()
+{
+    // toInt() alone returns 0 on a parse failure, not the default, and 0
+    // reaches a QTimer restarted on every keystroke: a typo here would have
+    // turned the debounce into a write per keystroke, uploaded by mbsync.
+    QTemporaryDir dir;
+    Config config;
+    config.load(writeIni(dir, QStringLiteral(
+        "[compose]\n"
+        "autosave_interval_ms=oops\n")));
+
+    QCOMPARE(config.compose().autosaveIntervalMs, 30000);
+    QVERIFY2(!config.problems().isEmpty(),
+             "a garbage autosave_interval_ms was accepted silently");
+}
+
+void TestConfig::garbageSendDelayIsRejectedNotZero()
+{
+    QTemporaryDir dir;
+    Config config;
+    config.load(writeIni(dir, QStringLiteral(
+        "[compose]\n"
+        "send_delay_ms=soon\n")));
+
+    QCOMPARE(config.compose().sendDelayMs, 5000);
+    QVERIFY2(!config.problems().isEmpty(),
+             "a garbage send_delay_ms was accepted silently");
+}
+
+void TestConfig::garbageAttachmentWarnBytesIsRejectedNotZero()
+{
+    // Verified against the actual defect: attachment_warn_bytes=banana gave 0
+    // via a bare toLongLong(), which would have warned about every attachment
+    // no matter how small.
+    QTemporaryDir dir;
+    Config config;
+    config.load(writeIni(dir, QStringLiteral(
+        "[compose]\n"
+        "attachment_warn_bytes=banana\n")));
+
+    QCOMPARE(config.compose().attachmentWarnBytes, qint64(26214400));
+    QVERIFY2(!config.problems().isEmpty(),
+             "a garbage attachment_warn_bytes was accepted silently");
+}
+
+void TestConfig::zeroOrNegativeAutosaveIntervalIsClamped()
+{
+    // Independent of the parse fix: a value that parses fine but is zero or
+    // negative must still not reach setInterval(), since nothing assigns a
+    // meaning to one, unlike mark_read_delay_ms's documented negative-means-off.
+    QTemporaryDir dir;
+    Config zero;
+    zero.load(writeIni(dir, QStringLiteral(
+        "[compose]\n"
+        "autosave_interval_ms=0\n")));
+    QVERIFY2(zero.compose().autosaveIntervalMs >= 1000,
+             qPrintable(QStringLiteral("zero autosave interval was not clamped: %1")
+                            .arg(zero.compose().autosaveIntervalMs)));
+
+    QTemporaryDir dir2;
+    Config negative;
+    negative.load(writeIni(dir2, QStringLiteral(
+        "[compose]\n"
+        "autosave_interval_ms=-500\n")));
+    QVERIFY2(negative.compose().autosaveIntervalMs >= 1000,
+             qPrintable(QStringLiteral("negative autosave interval was not clamped: %1")
+                            .arg(negative.compose().autosaveIntervalMs)));
+}
+
+void TestConfig::unrecognisedQuotePositionWarnsAndFallsBackToAbove()
+{
+    // Matches the precedent set by sync_on_exit, language and date_format:
+    // the only silent fallbacks in this file are for ABSENT keys, never for
+    // malformed ones.
+    QTemporaryDir dir;
+    Config config;
+    config.load(writeIni(dir, QStringLiteral(
+        "[compose]\n"
+        "quote_position=abov\n")));
+
+    QVERIFY2(config.compose().quotePosition == ComposeSettings::QuotePosition::Above,
+             "an unrecognised quote_position must still fall back to Above");
+    QVERIFY2(!config.problems().isEmpty(),
+             "an unrecognised quote_position was accepted silently");
+}
+
 QTEST_MAIN(TestConfig)
 #include "test_config.moc"
diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts
index edc9d5e..4dbc62d 100644
--- a/translations/qtmaildir_it_IT.ts
+++ b/translations/qtmaildir_it_IT.ts
@@ -48,6 +48,22 @@
         <translation>L&apos;account &apos;%1&apos; non ha un cestino configurato; aggiungere una chiave &apos;trash&apos; alla sua sezione. L&apos;eliminazione non funzionerà per questo account finché non verrà fatto.</translation>
     </message>
     <message>
+        <source>[compose] quote_position &apos;%1&apos; is not recognised; expected above or below. Using above.</source>
+        <translation>[compose] quote_position &apos;%1&apos; non è riconosciuto; atteso above o below. Verrà usato above.</translation>
+    </message>
+    <message>
+        <source>[compose] autosave_interval_ms &apos;%1&apos; is not a number; using %2.</source>
+        <translation>[compose] autosave_interval_ms &apos;%1&apos; non è un numero; verrà usato %2.</translation>
+    </message>
+    <message>
+        <source>[compose] send_delay_ms &apos;%1&apos; is not a number; using %2.</source>
+        <translation>[compose] send_delay_ms &apos;%1&apos; non è un numero; verrà usato %2.</translation>
+    </message>
+    <message>
+        <source>[compose] attachment_warn_bytes &apos;%1&apos; is not a number; using %2.</source>
+        <translation>[compose] attachment_warn_bytes &apos;%1&apos; non è un numero; verrà usato %2.</translation>
+    </message>
+    <message>
         <source>Startup account &apos;%1&apos; is not a configured account; starting on all accounts.</source>
         <translation>L&apos;account iniziale &apos;%1&apos; non è un account configurato; si parte da tutti gli account.</translation>
     </message>