fix: sanitise untrusted contact names on insertionHEADmaster

A vCard FN is untrusted and ContactStore faithfully decodes `\n` to a real
newline, so `Evil\nBcc: x` was inserted raw into a recipient field and flowed
through splitRecipients() to MessageBuilder. contactInsertionText() only quoted
a name carrying a comma or a double quote, so every other RFC 5322 special
(<, >, ;, @) and any control character reached the header unguarded.

The name now has control characters and whitespace runs replaced by single
spaces, and every non-empty name is quoted, with `\` escaped before `"`.
Quoting contains all the specials in one step. The parser is left faithful;
this is fixed at the consumer/trust boundary.

Tests: a name with a decoded newline inserts no control character and yields
one recipient; a name with <, >, ;, @ is quoted and yields one recipient. The
three tests that expected an unquoted plain name now expect the quoted form.

2 files changed, 100 insertions, 23 deletions

diff --git a/src/composewindow.cpp b/src/composewindow.cpp
index f62f38f..eb983d6 100644
--- a/src/composewindow.cpp
+++ b/src/composewindow.cpp
@@ -124,26 +124,34 @@ QStringList splitRecipients(const QString &text)
 
 /// The text a completion inserts for \p contact.
 ///
-/// A display name containing a comma must be QUOTED, because the recipient
-/// fields are comma-separated and splitRecipients() would otherwise cut the
-/// name in half before the message was built. A name containing a double quote
-/// is quoted too, with its quotes backslash-escaped: an unquoted `"` in an
-/// address header is malformed. The backslash is escaped FIRST, or escaping the
-/// quote would then double the backslashes it just introduced.
+/// UNTRUSTED INPUT ARRIVES HERE. A display name comes from a vCard's FN, and
+/// ContactStore::unescapeText() faithfully decodes `\n` to a real newline, so a
+/// hostile card can carry `Evil\nBcc: x` and be inserted raw into a recipient
+/// header. The name is sanitised and then ALWAYS quoted when non-empty, rather
+/// than only for a comma or a quote: quoting contains every RFC 5322 special
+/// (`<`, `>`, `;`, `@`, spaces, commas) in one step, and escaping `\` before `"`
+/// is what makes the quoted form valid. Control characters become single spaces
+/// and runs collapse, so a decoded newline cannot forge a header line.
 QString contactInsertionText(const Contact &contact)
 {
-    if (contact.name.isEmpty())
-        return contact.email;
-
-    if (contact.name.contains(QLatin1Char(','))
-        || contact.name.contains(QLatin1Char('"'))) {
-        QString escaped = contact.name;
-        escaped.replace(QLatin1Char('\\'), QStringLiteral("\\\\"));
-        escaped.replace(QLatin1Char('"'), QStringLiteral("\\\""));
-        return QStringLiteral("\"%1\" <%2>").arg(escaped, contact.email);
+    QString name;
+    name.reserve(contact.name.size());
+    for (const QChar c : contact.name) {
+        if (c.isSpace() || c.category() == QChar::Other_Control) {
+            if (!name.isEmpty() && !name.endsWith(QLatin1Char(' ')))
+                name.append(QLatin1Char(' '));
+            continue;
+        }
+        name.append(c);
     }
+    name = name.trimmed();
+
+    if (name.isEmpty())
+        return contact.email;
 
-    return QStringLiteral("%1 <%2>").arg(contact.name, contact.email);
+    name.replace(QLatin1Char('\\'), QStringLiteral("\\\\"));
+    name.replace(QLatin1Char('"'), QStringLiteral("\\\""));
+    return QStringLiteral("\"%1\" <%2>").arg(name, contact.email);
 }
 
 /// The span an accepted completion replaces: the comma-delimited token the
diff --git a/tests/test_composewindow.cpp b/tests/test_composewindow.cpp
index e2673b7..bedd793 100644
--- a/tests/test_composewindow.cpp
+++ b/tests/test_composewindow.cpp
@@ -114,6 +114,8 @@ private slots:
     void completionStillWorksAfterAComma();
     void insertingACommaNameQuotesItAndKeepsOneRecipient();
     void insertingANameWithAQuoteEscapesIt();
+    void insertingANameWithAControlCharacterStripsIt();
+    void insertingANameWithHeaderSpecialsQuotesIt();
     void anEmptyNameInsertsTheBareAddress();
     void noContactsLeavesTheBehaviourUnchanged();
 
@@ -1222,10 +1224,11 @@ void TestComposeWindow::completionOffersAContactOnTheFirstRecipient()
     QVERIFY2(contactPopup() && contactPopup()->isVisible(),
              "typing a contact's name must offer it");
     QCOMPARE(contactPopup()->model()->index(0, 0).data(Qt::DisplayRole).toString(),
-             QStringLiteral("Alice Example <alice@example.org>"));
+             QStringLiteral("\"Alice Example\" <alice@example.org>"));
 
     acceptFirstPopupRow();
-    QCOMPARE(to->text(), QStringLiteral("Alice Example <alice@example.org>"));
+    QCOMPARE(to->text(),
+             QStringLiteral("\"Alice Example\" <alice@example.org>"));
 }
 
 /// The candidate is matched on the ADDRESS as well as the name. A single
@@ -1253,10 +1256,11 @@ void TestComposeWindow::completionMatchesTheAddressAsWellAsTheName()
     QVERIFY2(contactPopup() && contactPopup()->isVisible(),
              "typing an address must offer the contact");
     QCOMPARE(contactPopup()->model()->index(0, 0).data(Qt::DisplayRole).toString(),
-             QStringLiteral("Alice Example <alice@example.org>"));
+             QStringLiteral("\"Alice Example\" <alice@example.org>"));
 
     acceptFirstPopupRow();
-    QCOMPARE(to->text(), QStringLiteral("Alice Example <alice@example.org>"));
+    QCOMPARE(to->text(),
+             QStringLiteral("\"Alice Example\" <alice@example.org>"));
 }
 
 /// The case this whole task exists for: `setCompleter()` would set the
@@ -1286,11 +1290,13 @@ void TestComposeWindow::completionStillWorksAfterAComma()
     // Bob, not Alice: the prefix is the token after the comma, so Alice must
     // not be offered any more.
     QCOMPARE(contactPopup()->model()->index(0, 0).data(Qt::DisplayRole).toString(),
-             QStringLiteral("Bob Example <bob@example.org>"));
+             QStringLiteral("\"Bob Example\" <bob@example.org>"));
 
     acceptFirstPopupRow();
-    QCOMPARE(to->text(),
-             QStringLiteral("alice@example.org, Bob Example <bob@example.org>"));
+    QCOMPARE(
+        to->text(),
+        QStringLiteral(
+            "alice@example.org, \"Bob Example\" <bob@example.org>"));
 }
 
 /// A display name containing a comma must be QUOTED on insertion, or
@@ -1360,6 +1366,69 @@ void TestComposeWindow::insertingANameWithAQuoteEscapesIt()
     QCOMPARE(message.to.size(), 1);
 }
 
+/// FN is untrusted and ContactStore faithfully decodes `\n` to a real newline,
+/// so a hostile card can try to smuggle a header line through a display name.
+/// Nothing the completion inserts may carry a control character.
+void TestComposeWindow::insertingANameWithAControlCharacterStripsIt()
+{
+    const Config config = configWithDrafts();
+
+    ComposeContext context;
+    context.kind = ComposeContext::Kind::New;
+    context.accountKey = QStringLiteral("work");
+
+    ComposeWindow window(context, config, m_dir->path());
+    window.setContacts({ { QStringLiteral("Evil\nBcc: x"),
+                           QStringLiteral("evil@example.org") } });
+    window.show();
+    QVERIFY(QTest::qWaitForWindowExposed(&window));
+
+    auto *to = window.findChild<QLineEdit *>(QStringLiteral("to"));
+    QVERIFY(to);
+    to->setFocus();
+
+    QTest::keyClicks(to, QStringLiteral("Evil"));
+    QVERIFY(contactPopup() && contactPopup()->isVisible());
+    acceptFirstPopupRow();
+
+    for (const QChar c : to->text())
+        QVERIFY2(c.category() != QChar::Other_Control,
+                 "the inserted name must carry no control character");
+
+    QCOMPARE(to->text(),
+             QStringLiteral("\"Evil Bcc: x\" <evil@example.org>"));
+    QCOMPARE(window.currentMessage().to.size(), 1);
+}
+
+/// `<`, `>`, `;` and `@` are RFC 5322 specials. Quoting the whole name keeps
+/// every one of them inside the display name instead of in the header grammar.
+void TestComposeWindow::insertingANameWithHeaderSpecialsQuotesIt()
+{
+    const Config config = configWithDrafts();
+
+    ComposeContext context;
+    context.kind = ComposeContext::Kind::New;
+    context.accountKey = QStringLiteral("work");
+
+    ComposeWindow window(context, config, m_dir->path());
+    window.setContacts({ { QStringLiteral("Weird <Name>; @here"),
+                           QStringLiteral("weird@example.org") } });
+    window.show();
+    QVERIFY(QTest::qWaitForWindowExposed(&window));
+
+    auto *to = window.findChild<QLineEdit *>(QStringLiteral("to"));
+    QVERIFY(to);
+    to->setFocus();
+
+    QTest::keyClicks(to, QStringLiteral("Weird"));
+    QVERIFY(contactPopup() && contactPopup()->isVisible());
+    acceptFirstPopupRow();
+
+    QCOMPARE(to->text(),
+             QStringLiteral("\"Weird <Name>; @here\" <weird@example.org>"));
+    QCOMPARE(window.currentMessage().to.size(), 1);
+}
+
 /// A card with an address but no name completes on the address alone, with no
 /// empty angle brackets.
 void TestComposeWindow::anEmptyNameInsertsTheBareAddress()